feat(resizing): Add new performant useResizable hook #91548
Conversation
github-actions
bot
added
the
Scope: Frontend
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
| /** | ||
| * 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; |
There was a problem hiding this comment.
react aria kinda does these buttonProps it might make sense to have a handleProps if we think they should be applied in a uniform way
| const startXRef = useRef<number>(0); | ||
| const startWidthRef = useRef<number>(0); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
might make sense to be a useLayoutEffect so it doesn't appear as the smaller size then the bigger size
| ? parseInt(localStorage.getItem(sizeStorageKey) ?? '', 10) | ||
| : undefined; | ||
|
|
||
| ref.current.style.width = `${storedSize ?? initialSize}px`; |
There was a problem hiding this comment.
i think whats difficult about using px values here is what if i resize it while i'm on my big monitor then open it again with a smaller window. could it be too large?
There was a problem hiding this comment.
you might consider storing the localstorage value as a % and computing the px value
There was a problem hiding this comment.
might depend on what maxValue is for these usecases
|
@MichaelSun48 can we instead modify the old hook? I would be happy to help you with this and I have even started some work around it. The reason I am proposing this is because what you have here is exactly how the old hook used to work (I know bc I wrote it initially), but then people ended up adding size state storage to it without me knowing, which made it perform poorly. If you want, I'd be happy to team up on this and make sure that we only have resizable hook and behaviour in our code. |
|
This is the change in question. It is almost there, but requires me to make a few additional updates to support how size clearing works. |
|
@JonasBa yeah for sure! I intended to replace all callsites of the old hook with this new one, but it seems like you've got a headstart on that already in your PR. Should I just take down this PR and help update yours in that case? |
|
I think I would prefer if we change the old hook. Fwiw, that PR of mine cannot be merged yet, as some functionality like the double click and reset is broken in a few places - the usage of this is not tested. Let me put something on the calendar for us this week to look at what needs changing, I'm pretty confident we can get it done quickly |
Creates a new hook
useResizableuseResizableconsumes 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.Next steps would be to support vertical resizing, and replace as many usages of
useResizableDrawerwith this.Stories entry incoming