Allow multiple selection in SelectNext

[acelaya]

This PR extends SelectNext so that it supports multiple items to be selected at once.

It does it by allowing value to be an array of items instead of a single item. When that happens, items in the list do not set themselves as selected, but they toggle themselves from that list instead.

Additionally, it adds a new multiple prop to be passed, which causes the listbox to be kept open when selecting options from the list.

State management is delegated to consumers, simplifying the implementation.

The implementation is inspired in how HeadlessUI does it.

multi-select-2024-07-03_09.08.04.mp4

TODO

• Add tests
• Check if there's any a11y implication to take into consideration.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b9b6ce5) to head (576e773).

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1601   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           62        62           
  Lines         1047      1066   +19     
  Branches       402       410    +8     
=========================================
+ Hits          1047      1066   +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[acelaya]

This is a bit opinionated and not entirely obvious behavior.

Perhaps a better option would be to allow <SelectOption /> to provide its own onChange onSelect, in which case it gets invoked instead of the default selectValue(...) logic.

[robertknight]

I suggested above that it might help to avoid errors if the component just threw an error if in multi-select mode and a non-array value is passed.

[acelaya]

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-multiselectable

[robertknight]

I experimented with making multiple part of the SingleValueProps and MultiValueProps types so that TS can infer the types of value/onChange as appropriate. It works, except that it turned out to be necessary to explicitly specify the generic type for SelectNext rather than have TS infer it. That's quite an unusual thing to do, so it is probably better if that is not required.

diff --git a/src/components/input/SelectNext.tsx b/src/components/input/SelectNext.tsx
index 89762a4..c5b500c 100644
--- a/src/components/input/SelectNext.tsx
+++ b/src/components/input/SelectNext.tsx
@@ -246,11 +246,13 @@ function useListboxPositioning(
 type SingleValueProps<T> = {
   value: T;
   onChange: (newValue: T) => void;
+  multiple: false | undefined;
 };
 
 type MultiValueProps<T> = {
   value: T[];
   onChange: (newValue: T[]) => void;
+  multiple: true;
 };
 
 export type SelectProps<T> = CompositeProps &
@@ -258,13 +260,6 @@ export type SelectProps<T> = CompositeProps &
     buttonContent?: ComponentChildren;
     disabled?: boolean;
 
-    /**
-     * Whether this select should allow multi-selection or not.
-     * When this is true, the listbox is kept open when an option is selected.
-     * Defaults to false.
-     */
-    multiple?: boolean;
-
     /**
      * `id` attribute for the toggle button. This is useful to associate a label
      * with the control.
diff --git a/src/pattern-library/examples/select-next-multiple.tsx b/src/pattern-library/examples/select-next-multiple.tsx
index 9034b4b..90b2778 100644
--- a/src/pattern-library/examples/select-next-multiple.tsx
+++ b/src/pattern-library/examples/select-next-multiple.tsx
@@ -22,7 +22,7 @@ export default function App() {
   return (
     <div className="w-96 mx-auto">
       <label htmlFor={selectId}>Select students</label>
-      <SelectNext
+      <SelectNext<ItemType>
         multiple
         value={values}
         onChange={setSelected}

[robertknight]

This looks good and accessibility works well with VoiceOver. I suggested that rather than trying to handle values being a non-array when multiple is true in some undocumented way, the component could simply throw an error.

[robertknight]

I suggested above that it might help to avoid errors if the component just threw an error if in multi-select mode and a non-array value is passed.

[robertknight]

I noticed a possible hazard if single-selection is used but a user tried to use a tuple type as the item.

[robertknight]

LGTM. I do have a UX question over whether clicking a reset option should close the listbox automatically, rather than keep it open as clicking other options does. I suggest we ship this and then discuss that.

[acelaya]

I do have a UX question over whether clicking a reset option should close the listbox automatically, rather than keep it open as clicking other options does

Yep, I thought the same.