feat(multiselect): new All option for multiselect #16236
Conversation
Gururajj77
changed the title
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Hey @Gururajj77 and @preetibansalui
Awesome new functionality! it is working perfect 🚀
Just a couple things I noticed
| @@ -226,6 +227,8 @@ export interface MultiSelectProps<ItemType> | |||
| */ | |||
| items: ItemType[]; | |||
|
|
|||
| itemsWithSelectAll: ItemType[]; | |||
There was a problem hiding this comment.
Add to MultiSelect.propTypes and also would good to insert a description, so when the user reads the Overview page it has more context.
| /** | ||
| * to Show/Hide All option | ||
| */ | ||
| selectAll?: boolean; |
There was a problem hiding this comment.
Add to MultiSelect.propTypes
| const itemsWithSelectAll = selectAll | ||
| ? [selectAllOption, ...filteredItems] | ||
| : filteredItems; |
There was a problem hiding this comment.
How does this work when the component is controlled via selectedItems? Does the user get the selectAllOption in the items returned through onChange? Should they get that back? How would someone using the controlled component check the select all, or mark it indeterminate? Do they need to or does it happen automatically?
It might be worth adding a controlled multiselect select-all example to storybook to discover the answers to some of these things, if we need to make some workarounds, and/or what to document in storybook.
If there are unavoidable prop conflicts, like selectAll just plain doesn't work with the component in a controlled state, then ideally we'd flag this with a warning in the dev console.
e.g.: "MultiSelect's
selectAllprop does not work when in a controlled state usingselectItems"
There was a problem hiding this comment.
Hi Taylor, Please find the response:
Does the user get the selectAllOption in the items returned through onChange?
Yes user will get selectAllOption in the items returned through onChange.
Should they get that back?
It's the part of Items now.
How would someone using the controlled component check the select all, or mark it indeterminate? Do they need to or does it happen automatically?
If they are using controlled component, they won't have access to selectAll or indeterminate state. To enable these things user has to enable 'hasSelectAll' as true.
If there are unavoidable prop conflicts, like selectAll just plain doesn't work with the component in a controlled state, then ideally we'd flag this with a warning in the dev console.
If we are adding the prop 'hasSelectAll' prop in controlled state, it will work simply as the default one. Only issue is we can have conflict of logics, for eg - the current controlled component is selecting disabled items as well while we are not.
While writing these responses, I found that we have to disable select All functionality in case of controlled state. User can either use selectedItems or hasSelectAll prop. We will add a console warning as suggested.
| const selectedItemsWithoutSelectAll = selectedItems.filter( | ||
| (item: any) => item.id !== 'select-all-option' | ||
| ); |
There was a problem hiding this comment.
Is there are more reliable way to determine this? Thinking about how some consumers might already be rolling their own version of this feature and using this same id. It might be unavoidable, but thought I'd ask what you think.
There was a problem hiding this comment.
no we are using a new property in the selectAll object, called as selectAllFlag which just a boolean so that we don't mess with id being same as user's data.
| @@ -580,5 +557,34 @@ describe('MultiSelect', () => { | |||
| `${prefix}--list-box__wrapper--slug` | |||
| ); | |||
| }); | |||
|
|
|||
| it('should select all options when selectAll prop is true', async () => { | |||
There was a problem hiding this comment.
Along the same lines as the questions about controlled state - adding a test for how this is used with fully controlled selectedItems would be awesome. Select all, select none, indeterminate, etc.
There was a problem hiding this comment.
Hey just wanted to comment what we chatted about on the side - I'm still a bit concerned around the lack of compatibility with the component being fully controlled through selectedItems. The prop warning is a decent stopgap measure but I think we should also mark it as experimental. This way, if/when we get feedback that hasSelectAll needs to work with selectedItems we can make the necessary changes without having to do a breaking change.
Once marked experimental I think this is good to merge
Closes #16117
Select All feature for multiselect
Changelog
New
Changed