-
Notifications
You must be signed in to change notification settings - Fork 2
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
✨(react) rework the behavior of the Select component #166
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@openfun/cunningham-react": minor | ||
--- | ||
|
||
rework the behavior of the Select component |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" /> | ||
AntoLC marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit out of the scope, but I can see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I agree with you, we took the constraint to have the exact type as the native
|
||
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. | ||
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("<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( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this will cause:
-> If options change when typing for filtering it will display all options totally ignoring the filter. Which is a non-sense
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the options change when the select is open, the new options will not pass in
setOptionsToDisplay
, so actually if the user clicks on one of the option proposed, it will send back inonChange
an old value who could be not part of the new options. Depend the case it could potentially create a bug.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this is out the scope of this useEffect which is role is "Display all options only once when isOpen turns true". Adding here this new dep would result in the bug I described. It is some sort of event emitter "onOpen".
Regarding your use case we could do another PR to treat this potential issue which needs to be done in another part of the code, because this was existing before this PR.
In reality imo we cannot always apply the "add all used variables to the deps", in most cases the called methods use lots of other hidden variables that we never add as deps and that's ok. Sometimes it could cause bugs like the one I described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would have been nice to fix directly these 2 issues when we could do it directly:
But let's create another PR if you feel more confortable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I want to be able to make a release in between 😬