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

feat: multi-select hook for useSelect and useCombobox #879

Closed
Daniel15 opened this issue Jan 7, 2020 · 37 comments · Fixed by #895
Closed

feat: multi-select hook for useSelect and useCombobox #879

Daniel15 opened this issue Jan 7, 2020 · 37 comments · Fixed by #895

Comments

@Daniel15
Copy link

Daniel15 commented Jan 7, 2020

Is it possible to use the useSelect hook for a list that allows selecting multiple options? Is there an example of this? It seems like it only has one selectedItem prop that just takes a single item, so it is fine to manage the "currently selected items" state myself?

@silviuaavram
Copy link
Collaborator

Hi! Currently yes, multiple selection is something a user should do by himself. Having a stateful value of selectedValues and controlling useSelect prop of selectedItem and always passing it as null is probably the way to go right now.

A hook to support this out of the box is currently being developed, but it does not guarantee that will cover your use case, as it depends how you will display the selected items. The hook will support the scenario with selected item pills, for instance in the To: field of Gmail / Outlook, when you select a few people and can move between them using Left/Right Arrows, use Delete to remove from selection.

I will use this issue to track this, thanks for pointing it out! Will write the specs in a second comment.

@silviuaavram silviuaavram changed the title useSelect for multi-select feat: multi-select hook for useSelect and useCombobox Jan 7, 2020
@silviuaavram
Copy link
Collaborator

Long story short, I would like to support this:

Screenshot 2020-01-07 at 7 25 36 AM

useMultiple should be easily usable along useSelect or useCombobox. The hooks should be linked by a common ref element that should represent either the <input> or the <button> in order for focus management to work. Specs below:

  • getDropdownProps: should apply ArrowLeft handler that will move focus from dropdown to one of the items. Should be applied to either the button or the input, depending if dropdown is select or autocomplete.
  • getItemProps: should be applied to each selected item pill. 1) Will add ArrowLeft and ArrowRight handlers to provide navigation between items and back to the dropdown. 2) Should also apply tabIndex of 0 to the currently focused selected item. 3) Should also apply Delete handler to remove the selected item from selection.
  • getRemoveItemButton should be applied to the 'X' icons (if any) of the selected items. 1) Should apply onClick handler to remove the item from selection and move focus to another item / dropdown.

Also, an a11y message should be added each time an item has been removed from selection, either on Delete from item or on 'X' icon click.

@Daniel15
Copy link
Author

but it does not guarantee that will cover your use case, as it depends how you will display the selected items

For my use case, currently I have a dropdown list where the selected values are displayed on the button:

image

but I'm thinking of changing that. The thing is that currently all are selected by default, and I just show "All" in that case:
image

Maybe the "Gmail compose"-style pills would be fine for this use case. I'll play around with the UI a bit when I get a chance.

@rnnyrk
Copy link

rnnyrk commented Mar 13, 2020

Is there an ETA on the useMultiple hook? Really love the new implementation and would love to try it out (is there an beta / rc?) / help in any way!

@mtt87
Copy link

mtt87 commented Apr 1, 2020

I'm currently working on a multi select component and would love to try out and test it, I saw there is a PR open where a lot of work has already been done #895

@silviuaavram do you think it's safe to give it a try with the branch feat-use-multiple-selection or is there a published rc / alpha version?

Thanks 🙏

@silviuaavram
Copy link
Collaborator

silviuaavram commented Apr 1, 2020

It's already in 5.1.0-alpha.0 but I forgot to announce it here, my bad!

But I would like to make some API changes atm and will push them today. Will also release another alpha. Will let you know!

@silviuaavram
Copy link
Collaborator

Released 5.1.0-alpha.1 with the new changes!

@ncesar
Copy link

ncesar commented Apr 1, 2020

@silviuaavram where can I have access to 5.1.0-alpha.1?

@mtt87
Copy link

mtt87 commented Apr 1, 2020

@ncesar https://www.npmjs.com/package/downshift/v/5.1.0-alpha.1

npm i downshift@5.1.0-alpha.1 should do it ;)

@silviuaavram
Copy link
Collaborator

silviuaavram commented Apr 1, 2020 via email

@silviuaavram
Copy link
Collaborator

silviuaavram commented Apr 2, 2020

Also I would like to ask everyone to open issues on the new hook. It will get released so it's fine to create github issues based on it. Please leave feedback on API, new feature requests and bugs! The earlier the better. I will release this soon and then plan to iterate through all the hooks we have so far in order to improve them.

@silviuaavram
Copy link
Collaborator

Made some more changes on the docs, fixed TS and released https://www.npmjs.com/package/downshift/v/5.1.0-alpha.2.

@mtt87
Copy link

mtt87 commented Apr 2, 2020

Looks good I was able to create a component today
https://storybook-static.mtt.now.sh/?path=/story/multiselecttags--default

A few things I've noticed mostly about TS

onStateChange: ({ inputValue, type, selectedItem }) => {

Property 'type' does not exist on type 'Partial<UseComboboxState<Option>>'.

I defined a type for the value (Option) that I have in the items array

export type Option = {
  label: string
  value: string
}

// then TS is complaining here
onSelectedItemsChange: ({ selectedItems: s }: { selectedItems: Option[] }) => onItemsChange(s)
Type '({ selectedItems: s }: { selectedItems: Option[]; }) => void' is not assignable to type '(changes: Partial<UseMultipleSelectionState<Option>>) => void'.
  Types of parameters '__0' and 'changes' are incompatible.
    Type 'Partial<UseMultipleSelectionState<Option>>' is not assignable to type '{ selectedItems: Option[]; }'.
      Types of property 'selectedItems' are incompatible.
        Type 'Option[] | undefined' is not assignable to type 'Option[]'.
          Type 'undefined' is not assignable to type 'Option[]'.

@Daniel15
Copy link
Author

Daniel15 commented Apr 3, 2020

@silviuaavram While you're adjusting the TypeScript types, could you please make all the arrays readonly? I use readonly collections everywhere and had to add a hack in my code because Downshift doesn't use readonly arrays, even though AFAIK it doesn't directly mutate them: https://github.com/Daniel15/dnstools/blob/8d6f98ae943bc3ff32587f9289497690eec05520/src/DnsTools.Web/ClientApp/src/components/form/FormRowDropdownList.tsx#L25-L27

@silviuaavram
Copy link
Collaborator

silviuaavram commented Apr 3, 2020

interface Option {
  value: string
  id: string
}

const items: Option[] = [
  {value: 'test1', id: 'id1'}
]

export default function App() {
  useSelect<Option>({items, onSelectedItemChange: ({selectedItem}) => {
    console.log(selectedItem && selectedItem.value)
  }})
  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <h2>Start editing to see some magic happen!</h2>
    </div>
  );
}

For me this works. What am I missing?

@mtt87
Copy link

mtt87 commented Apr 3, 2020

@silviuaavram you are using useSelect rather than useMultipleSelection 😄

Anyway there are 2 issues

  1. Property 'type' does not exist on type 'Partial<UseComboboxState<Option>>'. when destructuring
const {
    isOpen,
    getMenuProps,
    getInputProps,
    getComboboxProps,
    highlightedIndex,
    getItemProps,
    selectItem,
    openMenu
  } = useCombobox({
    itemToString: (i: Option) => i?.label,
    inputValue: term,
    items,
    // TS error here
    onStateChange: ({ inputValue, type, selectedItem }) => {
      switch (type) {
        case useCombobox.stateChangeTypes.InputChange:
          setTerm(String(inputValue))

          break
        case useCombobox.stateChangeTypes.InputKeyDownEnter:
        case useCombobox.stateChangeTypes.ItemClick:
        case useCombobox.stateChangeTypes.InputBlur:
          if (selectedItem) {
            setTerm('')
            addSelectedItem(selectedItem)
            selectItem({ label: '', value: '' })
          }

          break
        default:
          break
      }
    }
  })
Type '({ selectedItems: s }: { selectedItems: Option[]; }) => void' is not assignable to type '(changes: Partial<UseMultipleSelectionState<Option>>) => void'.
  Types of parameters '__0' and 'changes' are incompatible.
    Type 'Partial<UseMultipleSelectionState<Option>>' is not assignable to type '{ selectedItems: Option[]; }'.
      Types of property 'selectedItems' are incompatible.
        Type 'Option[] | undefined' is not assignable to type 'Option[]'.
          Type 'undefined' is not assignable to type 'Option[]'.

here

  const {
    getSelectedItemProps,
    getDropdownProps,
    addSelectedItem,
    removeSelectedItem,
    selectedItems
  } = useMultipleSelection<Option>({
    initialSelectedItems: initialItems,
    // TS error here
    onSelectedItemsChange: ({ selectedItems: s }: { selectedItems: Option[] }) => onItemsChange(s)
  })

@silviuaavram
Copy link
Collaborator

Ok, so I see that the typings on those handlers need a bit of work.

type is not exposed on onStateChange or on any other on change related handlers. And on any hook.
Typings still have keysSoFar for useSelect but we migrated to inputValue instead. So that should be fixed as well.

I will handle this for useMultipleSelection in the current PR but I think the other ones for the other hooks can be fixed separately.

@mtt87
Copy link

mtt87 commented Apr 3, 2020

I think I found a bug useCombobox.stateChangeTypes.InputKeyDownEnter is never triggered if I type something in the input like "asd" and press enter.
It's only triggered if type, then press arrow down and select an element, then press enter.

onStateChange: ({ inputValue, type, selectedItem }) => {
      switch (type) {
        case useCombobox.stateChangeTypes.InputKeyDownEnter:
    }

Gonna check the source code now to see if I can catch where the issue is.
I'm also checking if this happens for useCombobox alone or if it's only related to the usage with useMultipleSelection

@silviuaavram
Copy link
Collaborator

If there is no selection it should not get triggered. The handlers are called only when state changes, the fact that it's InputKeyDownEnter is for your help to figure out better what triggered that state change.

@mtt87
Copy link

mtt87 commented Apr 3, 2020

@silviuaavram understood, thanks for clarifying

I'm trying to create a version that gives the user the option to create new items, similar to what gmail does, you can type hello and press enter and it creates a new value.

So far I've been able to do it like this but it's buggy because if I press a and then use the arrow down to select one of the filtered items and press enter, it will add 2 items: a and the item I've selected.
https://storybook-static.mtt.now.sh/?path=/story/multitagsselect--default

getDropdownProps({
  disabled,
  isOpen,
  onFocus: () => openMenu(),
  onKeyDownCapture: (e: KeyboardEvent) => {
    if (creatable && e.keyCode === 13) {
      const { value } = e.target as HTMLInputElement
      // don't do anything if it's empty
      if (!value) {
        return
      }
      // check if it doesn't exist yet
      if (!selectedItems.some((i: Option) => i.value === value)) {
        addSelectedItem({
          label: value,
          value
        })
        setTerm('')
      }
    }
  }
})

Do you have a suggestion? Is this the right approach?

Thank you

@silviuaavram
Copy link
Collaborator

Then you need to suppress one of the additions somehow. In the case above which one would you like to be added? Highlighted item or a new element based on the input value?

@silviuaavram
Copy link
Collaborator

silviuaavram commented Apr 3, 2020

If you want the highlighted item, then you can probably skip the onKeyDownCapture handler there based on something. And only add the item in your onSelectedItemChange or onStateChange (depends what you are using).

Not 100% sure what something can be. You can set up a flag in your onChange and based on that don't add the new item. You can probably use highlightedIndex from the useCombobox state and if that is >= 0 then you most probably added the highlighted element already. Or even better, check the filtered items length, and if that is 0 then you are good to go and add your new item.

What do you think?

@mtt87
Copy link

mtt87 commented Apr 3, 2020

Thanks for your reply, appreciate!

Not 100% sure what something can be.

That was exactly my issue today, I tried to find a way to not have to use the input manual handler 😄

You can set up a flag in your onChange and based on that don't add the new item. You can probably use highlightedIndex from the useCombobox state and if that is >= 0 then you most probably added the highlighted element already.

This might work

Or even better, check the filtered items length, and if that is 0 then you are good to go and add your new item.

This won't work because any value that's included in another will basically break this.
I.e. you have 2 options available: hello world and world then you want to add hello and it won't work because when you type hello based on the filtering function the dropdown will show hello world as possible option to select while your intention is to add hello only.

@silviuaavram
Copy link
Collaborator

#985 to fix the typings in useSelect and useCombobox. Can you take a look over? It also adds type to all handlers and removes the props from actionAndChanges passed to stateReducer.

@mtt87
Copy link

mtt87 commented Apr 4, 2020

#985 to fix the typings in useSelect and useCombobox. Can you take a look over? It also adds type to all handlers and removes the props from actionAndChanges passed to stateReducer.

I'll be able to check on Monday morning 😬

@mtt87
Copy link

mtt87 commented Apr 6, 2020

@silviuaavram
I was able to use highlightedIndex to fix my issue 🎉

onKeyDownCapture: (e: KeyboardEvent) => {
  if (creatable && e.keyCode === 13 && highlightedIndex === -1) {
  // user intention

#985 to fix the typings in useSelect and useCombobox. Can you take a look over? It also adds type to all handlers and removes the props from actionAndChanges passed to stateReducer.

I had a look it seems ok, best to publish it and I can try to see if all good in VSCode 👍

@mtt87
Copy link

mtt87 commented Apr 6, 2020

Found a possible issue, this is how to reproduce

  • Press arrow down and select an item from the combobox
  • Press enter, the item is selected and added correctly
  • The cursor is right after the item
    Screen Shot 2020-04-06 at 11 17 35 AM
  • Press backspace and the item is removed correctly

Bug:

  • Select with the mouse an item from the combobox and click
  • The item is correctly added
  • Now if you try to use backspace to remove the item you can't

I created a sandbox here to reproduce
https://codesandbox.io/s/ecstatic-driscoll-scdyc

@silviuaavram
Copy link
Collaborator

silviuaavram commented Apr 6, 2020 via email

@mtt87
Copy link

mtt87 commented Apr 6, 2020

That's expected right? You selected by mouse, why would the keyboard focus be in the input, probably it's on ?

I think the focus is indeed correct but if I manually click on the input now the focus is on the input and I would expect to be able to press backspace to remove the last added item and so on.

This doesn't happen right now, basically once you have selected an item with the mouse the only way to remove it is by pressing the X icon next to it as the backspace doesn't work anymore.

@silviuaavram
Copy link
Collaborator

isOpen is probably true. Did you check that? If it's true then kb handlers don't work.

I am still thinking about how to properly expose this as API. I should probably replace isOpen with a more explicit name that should not depend necessarily on isOpen, but to let the user decide when backspace/arrow navigation is permitted.

If I don't add anything and let the multiple kb navigation work always, it can get a bit messy. If you have item highlighted and press Arrow Left, useCombobox reacts on the blur event and selects the highlighted item. So you basically select highlightedItem + navigate on last selected item. Not really what you want I suppose.

I was inspired to use isOpen while checking how GMail does it, and in Gmail's To field, if you have the list of options open, you cannot delete by backspace or navigate by arrows.

@mtt87
Copy link

mtt87 commented Apr 7, 2020

Yes now that you tell me I realise that I open the dropdown when clicking on the input field so isOpen is true.
I can check tomorrow morning without that if it works correctly thank you 👍 😄

@silviuaavram
Copy link
Collaborator

Changed the isOpen to preventKeyAction as it makes better sense I think and released 5.1.0-beta.0 with the latest changes.

@mtt87
Copy link

mtt87 commented Apr 9, 2020

Tested now all seems good, didn't really notice any difference in my implementation 🤔
https://storybook-static.mtt.now.sh/?path=/story/multitagsselect--default

@silviuaavram
Copy link
Collaborator

I am pretty happy with the code so far. I expect this to be released tomorrow if there are no other issues. 5.1.0-beta.0 should contain the most up to date functional changes.

@silviuaavram
Copy link
Collaborator

🎉 This issue has been resolved in version 5.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tim-phillips
Copy link

@mtt87 do you have any code you can share for the components in that storybook link?

@mtt87
Copy link

mtt87 commented Apr 14, 2020

@tim-phillips If you are looking for a specific component I can share that with you but won't be able to share entire codebase as the repo is company private not open source.
They are built with Typescript, CSS-in-JS using https://rebassjs.org/ for the primitives and then a couple of custom components like the ListBox/MultiTagsSelect that are using Downshift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants