TagGroup: Add useTagGroupState and fix focus issues#3798
Conversation
|
Build successful! 🎉 |
reidbarber
changed the title
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Some typing suggestions/questions + a behavior change suggestion
|
Build successful! 🎉 |
|
Build successful! 🎉 |
ktabors
left a comment
ktabors
left a comment
There was a problem hiding this comment.
The testing says "previous" tag the code in stately says "next", unless there is no "next" then it is "previous".
| focusMode: 'cell' | ||
| }, state, tagRef); | ||
| // We want the group to handle keyboard navigation between tags. | ||
| delete rowProps.onKeyDownCapture; |
There was a problem hiding this comment.
rowProps.onKeyDownCapture is set in useGridListItem as onKeyDown(). I was wondering if this should be a replace with the onKeyDown you're defining and not a delete, but nothing consuming this will call onKeyDownCapture, right?
There was a problem hiding this comment.
We could set it to undefined down there, but it would do the same thing. And I think we want onKeyDown here, we're just removing that handler since it would handle focusing within a cell, which we don't want.
reidbarber
dismissed stale reviews from ktabors and snowystinger
via
d13c41c
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Just a couple of things of small things I noticed on this final pass but def can handle in a followup instead
| */ | ||
|
|
||
| export type {AriaGridListProps, GridListProps} from '@react-aria/gridlist'; | ||
| export type {ListState} from '@react-stately/list'; |
There was a problem hiding this comment.
The exports here for AriaGridListProps, GridListProps, and SpectrumListViewProps were kept in here to preserve back compat, but these props are now exported from their specific packages. IMO we can go ahead and just export ListState from @react-stately/list if needed, where did you need this type specifically?
There was a problem hiding this comment.
Good point, I went ahead and removed this export
| let onRemove = (key) => { | ||
| // If a tag is removed, restore focus to the tag after, or tag before if no tag after. | ||
| let restoreKey = state.collection.getKeyAfter(key) || state.collection.getKeyBefore(key); | ||
| state.selectionManager.setFocusedKey(restoreKey); |
There was a problem hiding this comment.
Can be follow up, but I just noticed that focus seems to be lost when deleting a tag via TalkBack, not sure why that is.
There was a problem hiding this comment.
We can investigate in a follow-up before release.
reidbarber
dismissed stale reviews from LFDanLu, snowystinger, and majornista
via
aec919e
6 participants
Closes #3070, #3074
✅ Pull Request Checklist:
📝 Test Instructions:
Open the TagGroup -> onRemove story. After removing a tag, focus should go to the next tag (or previous tag if no next tag) and you should still be able to keyboard navigate between tags using arrow keys.
🧢 Your Project: