TagGroup: add default empty state and support for custom empty state#4358
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| "hideButtonLabel": "Show less", | ||
| "actions": "Actions" | ||
| "actions": "Actions", | ||
| "noTags": "None" |
There was a problem hiding this comment.
Ran chromatic and it broke cuz it doesn't have ar-AE translations, wanna put a dummy string for noTags for ar-AE for now?
| onAction, | ||
| labelPosition | ||
| labelPosition, | ||
| renderEmptyState = () => <div>{stringFormatter.format('noTags')}</div> |
There was a problem hiding this comment.
Should we make this div the same height as a Tag? Right now the height of the container will shrink when the final tag is removed and replaced with this div.
There was a problem hiding this comment.
Maybe a min-height? To handle the case of a user having a longer string that wraps as their message, or putting something in there that takes up more height.
| {item.rendered} | ||
| </Tag> | ||
| ))} | ||
| {isEmpty && renderEmptyState()} |
There was a problem hiding this comment.
Noticed that focus is being lost to the body when all the tags are removed, is that going be handled in this PR or a separate one?
There was a problem hiding this comment.
It should now go to the container when the last tag is removed.
| } | ||
| ) | ||
| }> | ||
| <FocusRing focusRingClass={classNames(styles, 'focus-ring')}> |
There was a problem hiding this comment.
Not sure the FocusRing makes sense when ShowAll hides all items. The hiding all items in this story is an existing bug. Would this go away if we fix that bug and always show one tag?
|
Build successful! 🎉 |
| if (!lastTag.current) { | ||
| container.focus(); | ||
| } | ||
| }, 50); |
There was a problem hiding this comment.
why 50ms out of curiosity? why not immediately? I assume it's because the tag container doesn't get a tabIndex of -1 until the state change has taken effect? we could instead use a useEffect and a ref, the ref tracks previous count of tags and if the tag group has focus, if the count of tags goes from > 0 to === 0, then we focus the container
does focus get set to the container if we mouse click to remove the last tag already?
There was a problem hiding this comment.
The intention was to wait and check that the list tag actually got removed from the DOM. Mostly in order to avoid the case that it wasn't removed immediately (like a confirmation dialog before it actually gets removed). I think your suggestion is much better though, so I just changed it to that.
|
Build successful! 🎉 |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
LGTM, tested in chrome/safari/ff
|
Build successful! 🎉 |
| let prevCount = useRef(state.collection.size); | ||
| useEffect(() => { | ||
| if (prevCount.current > 0 && state.collection.size === 0 && isFocusWithin) { | ||
| ref.current.focus(); | ||
| } | ||
| }, [state.collection.size, isFocusWithin, ref]); |
There was a problem hiding this comment.
Should we update prevCount.current here at the end of the useEffect? Thinking about if the TagGroup goes from empty -> gets content -> becomes empty again (wouldn't focus the ref.current since prevCount never gets updated from initial render right?)
There was a problem hiding this comment.
eagle eyes, good catch, yes, this should always be updated at the end of the useEffect body regardless of anything else
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } @react-spectrum/tagTagGroup TagGroup<T extends {}> {
actionLabel?: string
onAction?: () => void
+ renderEmptyState?: () => JSX.Element
}SpectrumTagGroupProps SpectrumTagGroupProps<T> {
actionLabel?: string
onAction?: () => void
+ renderEmptyState?: () => JSX.Element
} |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
6 participants
Adds a default empty state of 'None', and support for a custom empty state via
renderEmptyState.If the empty state contains no focusable children, then the group itself should be focusable.
✅ Pull Request Checklist:
📝 Test Instructions:
Test new stories at TagGroup -> Empty state and Custom empty state
🧢 Your Project: