SelectMultiple - Change select all and clear all buttons to show when searching #6456
Conversation
There was a problem hiding this comment.
There is a scenario where I :
- search for "p"
- Select All selects "Apple", "Grape", and "Raspberry"
- clear search and search for "z"
Do we show a button here? I don't think so, but just asking to validate our current thinking.
There is a scenario where I :
- search for "p"
- Select All selects "Apple", "Grape", and "Raspberry"
- clear search and search for "w"
- Select All adds "Strawberry" and "Kiwi" to the existing selection, resulting in 5 options selected
- clear search and search for "b"
We show "Clear all" instead of "Select all" here. Is that what we want? I think so, but again just surveying edge cases.
Lastly, I think this PR would be a good candidate to improve our test scenario coverage. Can we add a Jest test which executes the second scenario above with assertions throughout?
| ? `Select all ${options.length} options` | ||
| : `${value?.length} options selected. Clear all?` | ||
| } | ||
| label={showSelectAll ? 'Select All' : 'Clear All'} |
There was a problem hiding this comment.
If "Select all matches" and "Clear all matches" is too verbose, two alternatives to explore:
- "Select matches" and "Clear matches"
<Button size="small" ... />could be reasonable. May need to implement as theme prop.
There was a problem hiding this comment.
Matt and I had an offline discussion about this one and agreed it could be handled in a separate issue. Changing to "Select matches" and "Clear matches" is still affecting the width of the drop container. I think the naming is clearer but we may want to look into sizing of the button, how comfortable are we with affecting the drop container width, etc. So I think this work could be handled separately
We don't show a button here since no matches were found after searching 'z'.
Yes, we show 'Clear All' whenever any options are in the search results and selected.
Good idea I realized I didn't add a test in my latest commit but I will go back and add a test for this |
|
Marking as draft, I need to go back and add a unit test |
What does this PR do?
Adjusts the behavior of SelectMultiple to show 'Select All' and 'Clear All' buttons when searching.
Note: In the original issue it was mentioned that the buttons should be labeled 'Select All Matches' and 'Clear All Matches' when the options are being filtered by a search term. When I tried this the button labels felt too long and they were taking up too much space within the SelectMultiple drop.
This PR also fixes an issue where selected options were bubbling up when searching. Options should only bubble on after the drop is closed an re-opened.
Where should the reviewer start?
What testing has been done on this PR?
How should this be manually tested?
Test with the existing SelectMultiple stories. Open the drop and type in the search. The 'Select All'/'Clear All' button should still be visible.
Do Jest tests follow these best practices?
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Closes #6444
Closes #6447
Screenshots (if appropriate)
Do the grommet docs need to be updated?
No
Should this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
Backwards compatible