[RovingFocusGroup] Refactor internals to take into account hidden items #618
Conversation
| /** | ||
| * Wraps an array around itself at a given start index | ||
| * Example: `wrapArray(['a', 'b', 'c', 'd'], 2) === ['c', 'd', 'a', 'b']` | ||
| */ | ||
| function wrapArray<T>(array: T[], startIndex: number) { | ||
| return array.map((_, index) => array[(startIndex + index) % array.length]); | ||
| } |
There was a problem hiding this comment.
This is duplicated from menuTypeahead.tsx. Let me know if you think we should do something about it.
There was a problem hiding this comment.
Super excited about this 🎉
I noticed one little thing when testing that felt a bit odd to me. If the items have been interacted with, and then the focused one is hidden/removed, it feels like we need to reset hasInteracted.
Happy to have a call about this one if i'm not making sense 🙂
What I see in the gif seems to make sense to me, so yeah perhaps we should have a call on that particular one! |
5116ae9 to
41ac090
Compare
There was a problem hiding this comment.
@jjenzz This is now ready for review again. The last commit contains the changes to @radix-ui/react-collection to use @radix-ui/react-slot.
I've left a few new comments/questions inline for you below.
Also, I'm pretty sure that we'll be able to reuse the collection we've just put in place in Menu and its items for the typeahead stuff instead of the custom hook. I think it'll help also to bring it inside the main file as part of the process, all in one place.
| const context = React.useContext(Context); | ||
|
|
||
| React.useEffect(() => { | ||
| context.itemMap.set(ref, { ref, ...((itemData as unknown) as ItemData) }); |
There was a problem hiding this comment.
had to do as unknow as for some reason once I added React.forwardRef
| >; | ||
|
|
||
| const RovingFocusGroup = React.forwardRef((props, forwardedRef) => { | ||
| const RovingFocusGroupImpl = React.forwardRef((props, forwardedRef) => { |
There was a problem hiding this comment.
Added extra Impl component so we can get access to the useCollection hook.
There was a problem hiding this comment.
I've left a comment above about the onEntryFocus naming but just something to consider, I'll leave it up to you as I don't want to hold this up because of it.
|
All good from me except this one is still there if that's the answer you were looking for? #618 (comment) |
Closes #581
This PR primarily refactors the internals of
RovingFocusGroupso we can take into account the fact that hidden items don't get focused by the browser.The crux of the implementation here is that we've introduced a wrapper as part of that API which will act as a proxy to send focus programatically to the correct place. We have a reference to all items in the group and so, we can programatically and progressively attempt to focus the "first" item, and eventually land on the correct one (the first visible one) without having to resort to calculating computed styles to know if an item is indeed hidden.
Once that was done, all of the dependent packages needed updated accordingly.
This has made the overall implementation(s) more declarative, and more the React way.
RadioGroupnow gets neworientation,dirandlooppropsToggleGroupnow gets neworientation,dirandlooppropscc @andy-hook, may be good for you to take a quick look through this to see if there's any impact on what you've been working on.