feat(nav, resizing): Make secondary nav resizible with new performant hook #91546
Conversation
github-actions
bot
added
the
Scope: Frontend
| width: 8px; | ||
| border-radius: 8px; | ||
| z-index: ${p => p.theme.zIndex.drawer + 2}; | ||
| cursor: ${p => (p.atMinWidth ? 'e-resize' : p.atMaxWidth ? 'w-resize' : 'ew-resize')}; |
There was a problem hiding this comment.
We could have useResizable consume the resize handle as a ref to apply these cursor styles. If we do that, though, then we should also just remove the onMouseDown and onDoubleClick handlers and apply that to the ref directly. This would make the hook significantly more opinionated, which is why I opted not to not have the hook handle.
| initialSize = RESIZABLE_DEFAULT_WIDTH, | ||
| maxWidth = RESIZABLE_MAX_WIDTH, | ||
| minWidth = RESIZABLE_MIN_WIDTH, |
There was a problem hiding this comment.
I considered making these non-optional parameters to force people to have an opinion on width config. Would love to get more thoughts on this.
| export const RESIZABLE_DEFAULT_WIDTH = 200; | ||
| export const RESIZABLE_MIN_WIDTH = 100; | ||
| export const RESIZABLE_MAX_WIDTH = Infinity; |
There was a problem hiding this comment.
For a hook like this I wouldn't even have defaults. make the callers set these.
| }: UseResizableOptions): { | ||
| /** | ||
| * Whether the drag handle is held. | ||
| */ | ||
| isHeld: boolean; | ||
| /** | ||
| * Apply this to the drag handle element to include 'reset' functionality. | ||
| */ | ||
| onDoubleClick: () => void; | ||
| /** | ||
| * Attach this to the drag handle element's onMouseDown handler. | ||
| */ | ||
| onMouseDown: (e: React.MouseEvent) => void; | ||
| /** | ||
| * The current size of the container. This is NOT updated during the drag | ||
| * event, only after the user finishes dragging. | ||
| */ | ||
| size: number; | ||
| } => { |
There was a problem hiding this comment.
Let's move this to an interface
interface UseResizeResult { ... }| initialSize = RESIZABLE_DEFAULT_WIDTH, | ||
| maxWidth = RESIZABLE_MAX_WIDTH, | ||
| minWidth = RESIZABLE_MIN_WIDTH, |
| * The ref to the element to be resized. | ||
| */ | ||
| ref: RefObject<HTMLElement | null>; | ||
|
|
There was a problem hiding this comment.
tiny nit: I usually don't put whitespace between properties. @TkDodo I actually wonder if there's a lint rule we can apply to this. Not sure if spacing is ever needed since we sort anyway.
| * If `sizeStorageKey` is provided and exists in local storage, | ||
| * then this will be ignored in favor of the size stored in local storage. | ||
| */ | ||
| initialSize?: number; |
There was a problem hiding this comment.
Feels like this shouldn't be something you set, the CSS should just imply this no?
| const handleMouseMove = useCallback( | ||
| (e: MouseEvent) => { | ||
| if (!isDraggingRef.current) return; | ||
|
|
||
| const deltaX = e.clientX - startXRef.current; | ||
| const newWidth = Math.max( | ||
| minWidth, | ||
| Math.min(maxWidth, startWidthRef.current + deltaX) | ||
| ); | ||
|
|
||
| if (ref.current) { | ||
| ref.current.style.width = `${newWidth}px`; | ||
| } | ||
| }, | ||
| [ref, minWidth, maxWidth] | ||
| ); |
There was a problem hiding this comment.
We need to use RequestAnimationFrame here. Otherwise this isn't performant
|
Separated the hook into its own PR. Will rebase and undraft this after that is merged |
Makes the secondary nav resizable via a new hook,
useResizable.useResizableconsumes a container ref and updates its width directly, rather than storing and returning asizestate like the previoususeResizableDrawer, making it feel significantly more performant. It also ensures that the cursor is always attached to the drag handle while dragging, which was an issue withuseResizableDrawer. Besides these changes, the API is similar touseResizableDrawerand supports params likeonResizeStart,onResizeEnd, and aisHeldreturn param.This hook only supports horizontal dragging, but it shouldn't be too hard to extend it to support vertical dragging as well.
Screen.Recording.2025-05-13.at.10.13.15.AM.mov