Improve syncing of the Combobox.Input value
#2042
Merged
+103
−161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…or externally
I also got rid of the manually dispatching of the change event if the
value changes from internally.
I think the correct mental model is:
- That the `Combobox.Input` value should change if the selected value
changes from the outside or from the inside.
- Note: It should _not_ do that if you are currently typing (once you
choose a new value it will re-sync, once you reset (escape / outside
click) it will also sync again).
- The `onChange`/`onInput` of the `Combobox.Input` itself should only be
called if you as the user type something. Not when the value is
"synced" based on the selected value. We were currently manually
dispatching events which works (to a certain extend) but smelled a bit
fishy to me.
The manual dispatching of events tried to solve an issue
(#1875), but I think
this can be solved in 2 other ways that make a bit more sense:
1. (Today) Use the `onBlur` on the input to reset the `query` value to filter
options.
2. (In the future) Use an exposed `onClose` (or similar) event to reset
your `query` value.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@RobinMalfait do you have an example for the mentioned |
|
Thank you a lot i was pulling my hair out because of this and i just had to install the newest version of the lib |
|
@RobinMalfait should i open a new issue for this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves the syncing of the
Combobox.Inputvalue.Syncing of the input should happen when the value changes internally or externally
I also got rid of the manually dispatching of the change event if the value changes from internally.
I think the correct mental model is:
Combobox.Inputvalue should change if the selected value changes from the outside (props) or from the inside.onChange/onInputof theCombobox.Inputitself should only be called if you as the user type something. Not when the value is "synced" based on the selected value.The manual dispatching of events tried to solve an issue (#1875), but I think this can be solved in 2 other ways that make a bit more sense:
onBluron the input to reset thequeryvalue to filter options. E.g.:<Combobox.Input onBlur={() => setQuery('')} />onClose(or similar) event to reset yourqueryvalue.Fixes: #1997