Skip to content
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

Merged
merged 57 commits into from
Dec 8, 2020
Merged

Single and Multi select #906

merged 57 commits into from
Dec 8, 2020

Conversation

wenche
Copy link
Contributor

@wenche wenche commented Nov 23, 2020

Resolve #896

@wenche wenche changed the title [WIP]: Single Select [WIP]: Single and Multi select Nov 25, 2020
@wenche
Copy link
Contributor Author

wenche commented Nov 25, 2020

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…

@wenche
Copy link
Contributor Author

wenche commented Nov 26, 2020

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? 🤔

@wenche
Copy link
Contributor Author

wenche commented Nov 26, 2020

I don't actually know where to put the ref element in this case. I'm also a bit worried about declaring it as a select element with SelectHTMLAttributes<HTMLSelectElement> in types. We don't actually have somewhere to spread the {…other} in this case as it's not actually a select element. What to do?

@wenche
Copy link
Contributor Author

wenche commented Nov 27, 2020

I'm not quite sure about the best way to solve a controlled version of selectedItems with multiple select.

@wenche
Copy link
Contributor Author

wenche commented Nov 30, 2020

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 SelectHTMLAttributes

@mimarz mimarz mentioned this pull request Dec 1, 2020
@wenche
Copy link
Contributor Author

wenche commented Dec 1, 2020

I attempted to support the itemToString prop as I thought it would be nice to have arrays with [{id: 'id', value: 'My string value}]. But it's complicated and I didn't managed to fulfill this within a reasonable amount of time. 😞

@wenche
Copy link
Contributor Author

wenche commented Dec 1, 2020

Still unsure if we should make items optional or not, my initial idea was to provide the opportunity to fetch on demand but it might be over engineering.

@wenche
Copy link
Contributor Author

wenche commented Dec 2, 2020

Add the{...other} on the input element. I'm still not quite sure about what is the correct approach here, as we are still using the SelectHTMLAttributes and what do we want people to do anyway 🤯

@wenche
Copy link
Contributor Author

wenche commented Dec 7, 2020

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.

@wenche
Copy link
Contributor Author

wenche commented Dec 7, 2020

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:

  • If the options are an array of object, you'll need to handle a string representation of that outside of Select and MultiSelect
  • Due to ARIA complexity, this version don't provide an indeterminate checkbox "Select all"
  • In a multi select with lot's of options, it might be difficult to quickly get an overview of the selected items.

@wenche wenche changed the title [WIP]: Single and Multi select Single and Multi select Dec 7, 2020
wenche added 14 commits December 7, 2020 11:38
Remove example of tokens/chip for each selected item and removing items from the list when they are selected
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
@wenche wenche marked this pull request as ready for review December 7, 2020 10:41
Copy link
Member

@vnys vnys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

wenche and others added 2 commits December 8, 2020 12:31
Don't actually understand why prop types are complaining here
Copy link
Contributor

@pomfrida pomfrida left a 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//setSelectedItem(changes.selectedItems)

) {
setSelectedItems(changes.selectedItems)

//setSelectedItem(changes.selectedItems)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//setSelectedItem(changes.selectedItems)

const getFilteredItems = (items: string[]) =>
items.filter((item) =>
// Remove selected items from the list
/* selectedItems.indexOf(item) < 0 && */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment

Suggested change
/* 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()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// item.toLowerCase().startsWith(inputValue.toLowerCase()),

onInputValueChange: ({ inputValue }) => {
setInputItems(
items.filter((item) =>
// item.toLowerCase().startsWith(inputValue.toLowerCase()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// item.toLowerCase().startsWith(inputValue.toLowerCase()),

@wenche wenche merged commit 0637bc2 into equinor:develop Dec 8, 2020
@wenche wenche deleted the select branch December 8, 2020 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create React Select component
3 participants