-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Single and Multi select #906
Conversation
There were some issues and bugs by using the whole wrapped checkbox with label for the multi select options list. Especially when you clicked several times at the same item. The example from the Downshift approach uses a single input for the option list, without the label. Since it's supposed to be good on a11y, it felt safer to follow the same approach 🤷♀️ It also seems like the click maniac bug disappeared… |
The ghost button doesn't actually fit, even if I killed some kittens and add a size of 24px. There's so much markup and styles inside it, so it's overdoing things in this scenario. Maybe we should consider to create a smaller variant of an icon button? 🤔 |
I don't actually know where to put the |
I'm not quite sure about the best way to solve a controlled version of |
I added a few examples to get a feeling on how it's like to use this in the wild. We need to discuss where we add spread props and what this component actually is in regard of element. I don't think we actually should use |
I attempted to support the |
Still unsure if we should make |
Add the |
We are struggling a bit with the ARIA representation of selected items, especially with MultiSelect. However, hopefully this will be fixed by the Downshift library as this is how I would expect it to work. |
This is v1 for single and multi select. As this is pretty complicated stuff, we don't want to make the scope too big. Known limitations:
|
Still using the useMultiselect hook in case we need the tokens in a distant future
To avoid a lot of duplicate declarations, let's try to reuse select tokens between single and multi
Like the text input field, it might be easier if we just use the input field without the wrapped label
This doesn't actually work very good with the ghost button, it has so much styles inside it and it's supposed to be 48px
Remove example of tokens/chip for each selected item and removing items from the list when they are selected
…ontrolled component if initialized with null
Assing 'null' to selectedItems didn't work very well with Downshift, so added some documentation on that.
It is hard to actually test the correct things for this element as aria-selected is not working the way you would expect it to do
getSelectedItemsProps should be used by rendered selected items, e.g. if we in a distant future render them as tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Co-authored-by: Frida Erdal <[email protected]>
Don't actually understand why prop types are complaining here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job. I just found some comments we can remove
) { | ||
setSelectedItems(changes.selectedItems) | ||
|
||
//setSelectedItem(changes.selectedItems) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//setSelectedItem(changes.selectedItems) | |
) { | ||
setSelectedItems(changes.selectedItems) | ||
|
||
//setSelectedItem(changes.selectedItems) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//setSelectedItem(changes.selectedItems) | |
const getFilteredItems = (items: string[]) => | ||
items.filter((item) => | ||
// Remove selected items from the list | ||
/* selectedItems.indexOf(item) < 0 && */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment
/* selectedItems.indexOf(item) < 0 && */ | |
/* selectedItems.indexOf(item) < 0 && */ | ||
|
||
// Can be used if we need filter the list on first letters aka starts with search | ||
// item.toLowerCase().startsWith(inputValue.toLowerCase()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// item.toLowerCase().startsWith(inputValue.toLowerCase()), | |
onInputValueChange: ({ inputValue }) => { | ||
setInputItems( | ||
items.filter((item) => | ||
// item.toLowerCase().startsWith(inputValue.toLowerCase()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// item.toLowerCase().startsWith(inputValue.toLowerCase()), | |
Resolve #896