hypothesis / frontend-shared

UI components and styles for Hypothesis front-end applications
https://patterns.hypothes.is
5 stars 2 forks source link

Allow multiple selection in SelectNext #1601

Closed acelaya closed 1 month ago

acelaya commented 1 month ago

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.

https://github.com/hypothesis/frontend-shared/assets/2719332/6c4a859b-5330-4652-80f0-ab6f9911a379

TODO

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1601 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 62 62 Lines 1047 1066 +19 Branches 402 410 +8 ========================================= + Hits 1047 1066 +19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

robertknight commented 1 month ago

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}
acelaya commented 1 month ago

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.