✨(react) rework the behavior of the Select component

We decided to change a bit the behavior of the Select:
- Do not trigger onChange on first render if value is defined
- Show all options on searchable select menu  opening even if there is an
  existing value.
- Clear the input field if no choice are selected
- Clear the added text to the input field when a value is already selected

.changeset/wicked-games-tie.md

@@ -0,0 +1,5 @@
+---
+"@openfun/cunningham-react": minor
+---
+
+rework the behavior of the Select component

packages/react/src/components/Forms/Select/mono-searchable.tsx

@@ -52,6 +52,25 @@ export const SelectMonoSearchable = (props: SubProps) => {
     downshiftReturn.selectItem(optionToSelect ?? null);
   }, [props.value, props.options, props.downshiftProps]);
 
+  // Even there is already a value selected, when opening the combobox menu we want to display all available choices.
+  useEffect(() => {
+    if (downshiftReturn.isOpen) {
+      setOptionsToDisplay(props.options);
+    }
+  }, [downshiftReturn.isOpen]);
+
+  const onInputBlur = () => {
+    setHasInputFocused(false);
+    if (downshiftReturn.selectedItem) {
+      // Here the goal is to make sure that when the input in blurred then the input value
+      // has exactly the selectedItem label. Which is not the case by default.
+      downshiftReturn.selectItem(downshiftReturn.selectedItem);
+    } else {
+      // We want the input to be empty when no item is selected.
+      downshiftReturn.setInputValue("");
+    }
+  };
+
   const inputProps = downshiftReturn.getInputProps({
     ref: inputRef,
     disabled: props.disabled,
@@ -82,7 +101,7 @@ export const SelectMonoSearchable = (props: SubProps) => {
           setHasInputFocused(true);
         }}
         onBlur={() => {
-          setHasInputFocused(false);
+          onInputBlur();
         }}
       />
     </SelectMonoAux>

packages/react/src/components/Forms/Select/mono.spec.tsx

@@ -8,11 +8,13 @@ import { CunninghamProvider } from ":/components/Provider";
 import {
   expectMenuToBeClosed,
   expectMenuToBeOpen,
+  expectNoOptions,
   expectOptions,
   expectOptionToBeDisabled,
   expectOptionToBeSelected,
   expectOptionToBeUnselected,
 } from ":/components/Forms/Select/test-utils";
+import { Input } from ":/components/Forms/Input";
 
 describe("<Select/>", () => {
   describe("Searchable", () => {
@@ -139,6 +141,141 @@ describe("<Select/>", () => {
 
       expect(input).toHaveValue("Paris");
     });
+    it("clears the text input if no item is selected", async () => {
+      const Wrapper = () => {
+        const [value, setValue] = useState<string | number | undefined>();
+        return (
+          <CunninghamProvider>
+            <div>Value = {value}|</div>
+            <Select
+              label="City"
+              options={[
+                {
+                  label: "Paris",
+                  value: "paris",
+                },
+                {
+                  label: "Panama",
+                  value: "panama",
+                },
+                {
+                  label: "London",
+                  value: "london",
+                },
+                {
+                  label: "New York",
+                  value: "new_york",
+                },
+                {
+                  label: "Tokyo",
+                  value: "tokyo",
+                },
+              ]}
+              searchable={true}
+              value={value}
+              onChange={(e) => setValue(e.target.value as any)}
+            />
+            <Input name="Something else" />
+          </CunninghamProvider>
+        );
+      };
+      render(<Wrapper />);
+
+      const user = userEvent.setup();
+      const input = screen.getByRole("combobox", {
+        name: "City",
+      });
+      screen.getByText("Value = |");
+      // It returns the input.
+      expect(input.tagName).toEqual("INPUT");
+
+      const menu: HTMLDivElement = screen.getByRole("listbox", {
+        name: "City",
+      });
+
+      expectMenuToBeClosed(menu);
+
+      // Click on the input.
+      await user.click(input);
+      expectMenuToBeOpen(menu);
+      expectOptions(["Paris", "Panama", "London", "New York", "Tokyo"]);
+
+      // Verify that filtering works.
+      await user.type(input, "Pa");
+      expectMenuToBeOpen(menu);
+      expectOptions(["Paris", "Panama"]);
+
+      await user.type(input, "rr");
+      expectNoOptions();
+
+      expect(input).toHaveValue("Parr");
+
+      // This is a way to blur the combobox.
+      await user.click(screen.getByRole("textbox"));
+
+      expect(input).toHaveValue("");
+      screen.getByText("Value = |");
+    });
+    it("clears the added text to the existing value input on blur if no other item is selected", async () => {
+      const Wrapper = () => {
+        const [value, setValue] = useState<string | number | undefined>(
+          "london",
+        );
+        return (
+          <CunninghamProvider>
+            <div>Value = {value}|</div>
+            <Select
+              label="City"
+              options={[
+                {
+                  label: "Paris",
+                  value: "paris",
+                },
+                {
+                  label: "Panama",
+                  value: "panama",
+                },
+                {
+                  label: "London",
+                  value: "london",
+                },
+                {
+                  label: "New York",
+                  value: "new_york",
+                },
+                {
+                  label: "Tokyo",
+                  value: "tokyo",
+                },
+              ]}
+              searchable={true}
+              value={value}
+              onChange={(e) => setValue(e.target.value as any)}
+            />
+            <Input name="Something else" />
+          </CunninghamProvider>
+        );
+      };
+      render(<Wrapper />);
+
+      const user = userEvent.setup();
+      const input = screen.getByRole("combobox", {
+        name: "City",
+      });
+      screen.getByText("Value = london|");
+      // It returns the input.
+      expect(input.tagName).toEqual("INPUT");
+
+      expect(input).toHaveValue("London");
+      await user.type(input, "rr");
+      expect(input).toHaveValue("Londonrr");
+
+      // This is a way to blur the combobox.
+      await user.click(screen.getByRole("textbox"));
+
+      expect(input).toHaveValue("London");
+      screen.getByText("Value = london|");
+    });
     it("clears value", async () => {
       render(
         <CunninghamProvider>
@@ -290,10 +427,12 @@ describe("<Select/>", () => {
         const [value, setValue] = useState<string | number | undefined>(
           "london",
         );
+        const [onChangeCounts, setOnChangeCounts] = useState(0);
         return (
           <CunninghamProvider>
             <div>
               <div>Value = {value}|</div>
+              <div>onChangeCounts = {onChangeCounts}|</div>
               <Button onClick={() => setValue(undefined)}>Clear</Button>
               <Button onClick={() => setValue("paris")}>Set Paris</Button>
               <Select
@@ -317,7 +456,10 @@ describe("<Select/>", () => {
                   },
                 ]}
                 value={value}
-                onChange={(e) => setValue(e.target.value as string)}
+                onChange={(e) => {
+                  setValue(e.target.value as string);
+                  setOnChangeCounts(onChangeCounts + 1);
+                }}
                 searchable={true}
               />
             </div>
@@ -332,6 +474,7 @@ describe("<Select/>", () => {
 
       // Make sure value is selected.
       screen.getByText("Value = london|");
+      screen.getByText("onChangeCounts = 0|");
 
       // Change value.
       const user = userEvent.setup();
@@ -343,14 +486,16 @@ describe("<Select/>", () => {
       });
       expectOptionToBeSelected(option);
 
-      // List should show only selected value.
+      // List should display all available options when re-opening.
-      expectOptions(["London"]);
+      expectOptions(["Paris", "London", "New York", "Tokyo"]);
 
       // Clear value.
       const button = screen.getByRole("button", {
         name: "Clear",
       });
       await user.click(button);
+      screen.getByText("Value = |");
+      await screen.findByText("onChangeCounts = 0|");
 
       // Select an option.
       await user.click(input);
@@ -362,6 +507,7 @@ describe("<Select/>", () => {
 
       // Make sure value is selected.
       screen.getByText("Value = new_york|");
+      screen.getByText("onChangeCounts = 1|");
 
       // clear value.
       await user.click(button);
@@ -377,6 +523,7 @@ describe("<Select/>", () => {
 
       screen.getByText("Value = paris|");
       expect(input).toHaveValue("Paris");
+      screen.getByText("onChangeCounts = 1|");
     });
     it("renders disabled", async () => {
       render(

packages/react/src/components/Forms/Select/mono.stories.tsx

@@ -84,7 +84,9 @@ export const Controlled = () => {
           label="Select a city"
           options={OPTIONS}
           value={value}
-          onChange={(e) => setValue(e.target.value as string)}
+          onChange={(e) => {
+            setValue(e.target.value as string);
+          }}
         />
         <Button onClick={() => setValue("")}>Reset</Button>
         <Button onClick={() => setValue(OPTIONS[0].value)}>
@@ -159,7 +161,9 @@ export const SearchableControlled = () => {
           options={OPTIONS}
           searchable={true}
           value={value}
-          onChange={(e) => setValue(e.target.value as string)}
+          onChange={(e) => {
+            setValue(e.target.value as string);
+          }}
         />
         <Button onClick={() => setValue("")}>Reset</Button>
         <Button onClick={() => setValue(OPTIONS[0].value)}>

packages/react/src/components/Forms/Select/mono.tsx

@@ -41,11 +41,19 @@ export const SelectMono = (props: SelectProps) => {
   const commonDownshiftProps: SubProps["downshiftProps"] = {
     initialSelectedItem: defaultSelectedItem,
     onSelectedItemChange: (e: UseSelectStateChange<Option>) => {
-      props.onChange?.({
+      const eventCmp = e.selectedItem ? optionToValue(e.selectedItem) : null;
-        target: {
+      const valueCmp = props.value ?? null;
-          value: e.selectedItem ? optionToValue(e.selectedItem) : undefined,
+      // We make sure to not trigger a onChange event if the value are not different.
-        },
+      // This could happen on first render when the component is controlled, the value will be
-      });
+      // set inside a useEffect down in SelectMonoSearchable or SelectMonoSimple. So that means the
+      // downshift component will always render empty the first time.
+      if (eventCmp !== valueCmp) {
+        props.onChange?.({
+          target: {
+            value: e.selectedItem ? optionToValue(e.selectedItem) : undefined,
+          },
+        });
+      }
     },
     isItemDisabled: (item) => !!item.disabled,
   };

packages/react/src/components/Forms/Select/test-utils.tsx

@@ -11,17 +11,18 @@ export const expectOptions = (expectedOptions: string[]) => {
     expect(option).toHaveTextContent(expectedOptions[i]);
   });
 };
+export const expectNoOptions = () => {
+  const options = screen.getAllByRole("listitem");
+  expect(options.length).toBe(1);
+  expect(options[0]).toHaveTextContent("No options available");
+};
 export const expectMenuToBeClosed = (menu: HTMLDivElement) => {
   expect(Array.from(menu.classList)).not.contains("c__select__menu--opened");
 };
 export const expectOptionToBeSelected = (option: HTMLLIElement) => {
-  expect(option).toHaveAttribute("aria-selected", "true");
   expect(Array.from(option.classList)).contains(
     "c__select__menu__item--selected",
   );
-  expect(Array.from(option.classList)).contains(
-    "c__select__menu__item--highlight",
-  );
 };
 export const expectOptionToBeUnselected = (option: HTMLLIElement) => {
   expect(option).toHaveAttribute("aria-selected", "false");