From 4e1d0f0bc78f3468695c630c2a3d0309186cd828 Mon Sep 17 00:00:00 2001 From: Nathan Vasse Date: Tue, 19 Sep 2023 17:15:58 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(react)=20rework=20the=20behavior=20of?= =?UTF-8?q?=20the=20Select=20component?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 5 + .../Forms/Select/mono-searchable.tsx | 21 ++- .../src/components/Forms/Select/mono.spec.tsx | 153 +++++++++++++++++- .../components/Forms/Select/mono.stories.tsx | 8 +- .../src/components/Forms/Select/mono.tsx | 18 ++- .../components/Forms/Select/test-utils.tsx | 9 +- 6 files changed, 199 insertions(+), 15 deletions(-) create mode 100644 .changeset/wicked-games-tie.md diff --git a/.changeset/wicked-games-tie.md b/.changeset/wicked-games-tie.md new file mode 100644 index 0000000..f7da4d7 --- /dev/null +++ b/.changeset/wicked-games-tie.md @@ -0,0 +1,5 @@ +--- +"@openfun/cunningham-react": minor +--- + +rework the behavior of the Select component diff --git a/packages/react/src/components/Forms/Select/mono-searchable.tsx b/packages/react/src/components/Forms/Select/mono-searchable.tsx index e419737..7db2206 100644 --- a/packages/react/src/components/Forms/Select/mono-searchable.tsx +++ b/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(); }} /> diff --git a/packages/react/src/components/Forms/Select/mono.spec.tsx b/packages/react/src/components/Forms/Select/mono.spec.tsx index 90902de..3faee95 100644 --- a/packages/react/src/components/Forms/Select/mono.spec.tsx +++ b/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("", () => { expect(input).toHaveValue("Paris"); }); + it("clears the text input if no item is selected", async () => { + const Wrapper = () => { + const [value, setValue] = useState(); + return ( + +
Value = {value}|
+ + + ); + }; + render(); + + 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( + "london", + ); + return ( + +
Value = {value}|
+ + + ); + }; + render(); + + 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( @@ -290,10 +427,12 @@ describe("", () => { }, ]} value={value} - onChange={(e) => setValue(e.target.value as string)} + onChange={(e) => { + setValue(e.target.value as string); + setOnChangeCounts(onChangeCounts + 1); + }} searchable={true} /> @@ -332,6 +474,7 @@ describe("", () => { }); expectOptionToBeSelected(option); - // List should show only selected value. - expectOptions(["London"]); + // List should display all available options when re-opening. + 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("", () => { screen.getByText("Value = paris|"); expect(input).toHaveValue("Paris"); + screen.getByText("onChangeCounts = 1|"); }); it("renders disabled", async () => { render( diff --git a/packages/react/src/components/Forms/Select/mono.stories.tsx b/packages/react/src/components/Forms/Select/mono.stories.tsx index c627e69..5120132 100644 --- a/packages/react/src/components/Forms/Select/mono.stories.tsx +++ b/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); + }} />