-
Notifications
You must be signed in to change notification settings - Fork 647
Draft for discussion: accept a persister for PageLayout.Pane #7348
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: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: c490d05 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 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 |
- Add onWidthChange prop to PageLayout.Pane for tracking width changes - Support controlled mode by syncing internal state from width prop changes - onWidthChange fires at drag end and double-click reset (not during drag) - Both onWidthChange and persister.save fire when both are provided - Add error handling for onWidthChange callback errors - Add comprehensive tests for new functionality
- Replace Record<string, never> with explicit NoPersistConfig type
- {persist: false} is more self-documenting than {}
- Make save required on WidthPersister (cleaner type)
- Rename isResizableWithoutPersistence to isNoPersistConfig
- Export NoPersistConfig type
- Update tests for new API
- Add PaneWidthValue type (PaneWidth | number | CustomWidthOptions) - When width is a number, use minWidth prop and viewport-based max - Export PaneWidthValue type and isNumericWidth helper - Add 'Resizable pane with number width' story - Update changeset documentation
d672433 to
c490d05
Compare
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/8993 |
Summary
Adds custom persistence options to
PageLayout.Pane'sresizableprop, addressing #7311.Problem
The current
resizable={true}implementation reads from localStorage synchronously during render, causing React hydration mismatches in SSR environments. Users have no way to:Solution
Extend the
resizableprop to accept configuration objects:Additional Enhancement: Number Width Support
The
widthprop now accepts numbers in addition to named sizes:Why accept a number?
Ergonomics for custom persistence - The primary use case involves React state. Without number support, you'd need to reconstruct a
CustomWidthOptionsobject on every width change, creating friction and potential for bugs.Leverages existing constraints - Numbers use the existing
minWidthprop (default 256px) and viewport-based max. This is what most consumers actually want.Consistency - Named sizes (
"medium") already use these same constraints. Numbers behave identically, just with an explicit pixel value.Potential risks considered
minWidthprop and viewport-based max.Math.max(min, Math.min(max, width))).isPaneWidth,isNumericWidth,isCustomWidthOptions), but this is hidden from consumers.API Design Decisions
Object config vs callback: Using
{persist: false}and{save: fn}rather than a single callback allows clear opt-out of persistence without needing a no-op function.Static module-level persister: The localStorage persister is a static object rather than recreated in hooks, avoiding issues with hook rules and memoization.
SaveOptions with widthStorageKey: Custom save functions receive the storage key, allowing consumers to namespace their persistence if needed.
New Exports
Types:
NoPersistConfig-{persist: false}CustomPersistConfig-{save: (width, options) => void | Promise<void>}SaveOptions-{widthStorageKey: string}ResizableConfig- Union of all resizable optionsPaneWidth-'small' | 'medium' | 'large'PaneWidthValue-PaneWidth | number | CustomWidthOptionsValues:
defaultPaneWidth-{small: 256, medium: 296, large: 320}Testing
usePaneWidthhookFixes #7311