✨(react) rework the behavior of the Select component #166
Conversation
🦋 Changeset detectedLatest commit: 5119875 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
It works perfect.
Just an observation, if I type "Pa" and click enter on the keyboard, the input stay on "Pa" even when I click somewhere else, but onChange is not trigger so it is ok, I don't know if you should do something or not.
Just a few comment then we are good.
| @@ -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.
A bit out of the scope, but I can see value can be from type string | number | string[] | undefined when Option.value can only be with string | undefined(https://github.com/openfun/cunningham/blob/main/packages/react/src/components/Forms/Select/mono.tsx#L9), I suppose they should have the same type and if possible infer from each other to avoid casting, what do you think ?
There was a problem hiding this comment.
Yes I agree with you, we took the constraint to have the exact type as the native <select/> which uses this union. However I agree that there is maybe something to work on on this.
- I think having
valuethat could only be of typestringis a mistake has it could be useful to allownumbertoo
| setOptionsToDisplay(props.options); | ||
| } | ||
| }, [downshiftReturn.isOpen]); | ||
|
|
There was a problem hiding this comment.
| }, [downshiftReturn.isOpen, props.options]); |
There was a problem hiding this comment.
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.
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 in onChange 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.
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.
It would have been nice to fix directly these 2 issues when we could do it directly:
-> If options change when typing for filtering it will display all options totally ignoring the filter.
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 in onChange an old value who could be not part of the new options. Depend the case it could potentially create a bug.
But let's create another PR if you feel more confortable.
There was a problem hiding this comment.
Yes I want to be able to make a release in between 😬
Really good catch! This was caused by the blur event not triggered by downshift in this situation which makes no sense as the user still have focus on input ... so to overcome this issue I added a straightforward fix-up after having tried to use the reducer 😅 |
| setOptionsToDisplay(props.options); | ||
| } | ||
| }, [downshiftReturn.isOpen]); | ||
|
|
There was a problem hiding this comment.
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 in onChange an old value who could be not part of the new options. Depend the case it could potentially create a bug.
NathanVss
force-pushed
the
feat/select-searchable-list
branch
from
fb9fc94 to
446e5c1
Compare
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
NathanVss
force-pushed
the
feat/select-searchable-list
branch
from
446e5c1 to
5119875
Compare
We decided to change a bit the behavior of the Select: