-
Notifications
You must be signed in to change notification settings - Fork 650
Refactor PageLayout.Pane resizable API: separate onWidthChange and currentWidth props #7449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: make-primer-react-pagelayout-accept-persister
Are you sure you want to change the base?
Refactor PageLayout.Pane resizable API: separate onWidthChange and currentWidth props #7449
Conversation
|
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
- Remove width from PersistConfig (no longer in resizable object) - Add onWidthChange callback prop to PageLayout.Pane - Add currentWidth prop to PageLayout.Pane for controlled width - Update usePaneWidth to accept onWidthChange and currentWidth - When onWidthChange is provided, it's called instead of any persistence - When currentWidth is provided, it overrides default width - Update stories to use new API pattern - Update tests to use currentWidth prop instead of resizable.width - All 73 tests passing Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
- Add 3 new tests for onWidthChange behavior - Test that onWidthChange is called instead of localStorage - Test that onWidthChange takes precedence over custom persist - Test that errors from onWidthChange are handled gracefully - Fix TypeScript error in test for removing currentWidth prop - All 76 tests passing, build succeeds Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
| * - `{persist: false}`: Enable resizing without any persistence | ||
| * - `{persist: 'localStorage'}`: Enable resizing with localStorage persistence (explicit) | ||
| * - `{persist: fn}`: Enable resizing with custom persistence function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot we should remove these options now for resizable, which is only a boolean or undefind now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified resizable to just boolean | undefined in commit 1ffd338.
Removed:
PersistConfig,PersistFunction,SaveOptionstypesisPersistConfig,isCustomPersistFunction,isResizableEnabledhelpers- 22 tests related to persist config options
The API is now simpler:
resizable={true}- Enable resizing with localStorage (if noonWidthChange)resizable={false}or omit - Disable resizing- Use
onWidthChangeprop for custom persistence instead of{persist: fn}
- Change ResizableConfig from `boolean | PersistConfig` to `boolean | undefined`
- Remove PersistConfig, PersistFunction, SaveOptions types
- Remove isPersistConfig, isCustomPersistFunction, isResizableEnabled helpers
- Simplify usePaneWidth logic to only handle boolean resizable
- Update PageLayout.Pane docs to reflect simpler API
- Update story using {persist: false} to just use resizable
- Remove tests for PersistConfig and custom persist functions
- Update remaining tests to use boolean resizable
- Remove exports of removed types from index.ts
- All 54 tests passing, build succeeds
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Refactors
PageLayout.Paneresizable configuration to use separateonWidthChangecallback andcurrentWidthprops, and simplifiesresizableto a boolean.Before:
After:
Changelog
New
PageLayout.Pane:currentWidth?: numberprop for controlled widthPageLayout.Pane:onWidthChange?: (width: number) => voidcallback for width changesChanged
resizableprop simplified to:boolean | undefined(no longer accepts configuration objects)onWidthChangeis provided, it takes precedence over localStorage persistenceRemoved
resizable.widthproperty (replaced bycurrentWidthprop)PersistConfigtype and configuration object pattern{persist: false | 'localStorage' | fn}PersistFunctionandSaveOptionstypesisPersistConfig,isCustomPersistFunction, andisResizableEnabledhelper functionsRollout strategy
Rationale: Adds new props and simplifies the API. Maintains backward compatibility for
resizable={true}but removes the configuration object pattern in favor of the simpleronWidthChangeprop for custom persistence.Testing & Reviewing
currentWidth+onWidthChangeonWidthChangeMerge checklist
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.