Fix Crash in useSelectableCollection when ref not attached to dom node in first render #1629
Conversation
…e in first render
|
Build successful! 🎉 |
| // If no default focus key is selected and the collection is mounted, focus the collection itself. | ||
| if (focusedKey == null && !shouldUseVirtualFocus && ref.current) { |
There was a problem hiding this comment.
Hm, this makes sense for stopping the error but there might be an issue since then focus never gets set on the ListBox itself (this useEffect only happens on the initial render). That would mean the focus remains on the select button itself and pressing up/down would open and close the select menu instead of navigating through the items in the list.
Perhaps this should be extracted to it's own useEffect? Maybe useListBox shouldn't be pulled out into the Select component? Maybe the useEffect should fire if ref.current updates? Not sure
There was a problem hiding this comment.
So I had some similar reservations, however, in practice it seems to work, though the example code didn't have any of the focus/selection code running yet.
I updated the example to match our code on our documentation page so that I could try out the concern that focus would remain on the button, basically a copy paste from https://react-spectrum.adobe.com/react-aria/useSelect.html#example and now it would appear that this check is unnecessary. It behaves with or without it.
I think the outcome of this is
this person can update their code to be more like our documentationEdit, they were trying to move it on purpose, so we'll still need to do Component css packages #2- we should still perform this check somewhere, but maybe some more thought should be put into it, i'll try some other configurations later hopefully this week
- figure out why their selection code isn't working
|
Build successful! 🎉 |
|
Closing because this only hides the problem |
Closes #1622
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: