Fix closing components using the transition prop, and after scrolling the page
#3407
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change should be temporary, and it will allow us to use the `useDidElementMove` with ref objects and direct `HTMLElement`s.
This change should be temporary, and it will allow us to use the `useResolveButtonType` hook with ref objects and direct `HTMLElement`s.
This change should be temporary, and it will allow us to use the `useRefocusableInput` hook with ref objects and direct `HTMLElement`s.
Accept `HTMLElement| null` instead of `MutableRefObject<HTMLElement | null>` in the `useTransition` hook.
So far we've been tracking the `button` and the the `items` DOM nodes in a ref. Typically, this is the way you do it, you keep track of it in a ref, later you can access it in a `useEffect` or similar by accessing the `ref.current`. There are some problems with this. There are places where we require the DOM element during render (for example when picking out the `.id` from the DOM node directly). Another issue is that we want to re-run some `useEffect`'s whenever the underlying DOM node changes. We currently work around that, but storing it directly in state would solve these issues because the component will re-render and we will have access to the new DOM node.
This doesn't support the `MutableRefObject<HTMLElement | null>` anymore.
We don't handle `MutableRefObject<HTMLElement | null>` anymore
Only accept `HTMLElement | null` instead of `MutableRefObject<HTMLElement | null>`
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
RobinMalfait
commented
Aug 1, 2024
RobinMalfait
changed the title
transition prop, and after scrolling the pagetransition prop, and after scrolling the page
thecrypticace
approved these changes
Aug 2, 2024
|
I like how much cleaner this makes some of the code 💯 |
This was referenced Sep 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where components such as the
MenuItemsand theListboxOptionswon't close correctly if these are used in combination with thetransitionprop and after you scrolled the page a little bit.For some background information: the
MenuandListboxcomponents can be used with thetransitionprop so that theMenuItemsandListboxOptionscan be transitioned in and out based on the open/closed state.To make the user experience better, we have some logic to check whether the
MenuButtonorListboxButtonmoved from its initial position. If it did move when it's closing, then we immediately stop all transitions by unmounting theMenuItemsorListboxOptionsimmediately.A situation this happens in:
Menu(with thetransitionprop)MenuItemswill fade out while moving around (while the page scrolls)anchorprop, then theMenuItemswill reposition itself which makes the UX even worse.While debugging this issue, I noticed a timing related issue because we are using the DOM element in a
useTransitionhook. Typically you store those DOM elements in auseRef. This is good practice because the DOM element will be filled in once you hit theuseEffecthook, and you don't introduce unnecessary re-renders.However, if the DOM element changes, then the
useRefvalue will update, but theuseEffecthook doesn't run again (because no state change happened).There are other places in Headless UI where we also need to re-run effects when the DOM node changes. There are also places where we need access to the DOM element during render.
One of React's rules is to not read / write a ref during render.
So, this PR simplifies a few components and hooks by actually storing the DOM elements in state directly instead of in a ref. This way we can use the DOM element in dependency arrays of
useEffecthooks, and we can use the DOM element during render.Maybe a bit of an unorthodox solution, but it works quite well and it simplifies the code quite a bit.
It might be easier to review commit by commit. The reason why is that in some hooks I added temporary support for
HTMLElement | nullin places where we wantedMutableRefObject<HTMLElement | null>. These now temporary look likeMutableRefObject<HTMLElement | null> | HTMLElement | null.This way we can use both versions while refactoring. By the end of the commits those are simplified again such that we only handle
HTMLElement | nullinstead ofMutableRefObject<HTMLElement | null>.Fixes: #3398
Fixes: #3403