ComboboxControl: add support for uncontrolled mode #42752
Merged
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
|
Regarding that test story.... I didn't add an Uncontrolled story because it felt like it would be a bit repetitive with it being so similar to the current story. The interaction would be pretty much identical. That said, if others think it would be beneficial, I'm happy to implement a new story on this PR as well... I can just swap out the logged value for a printed output the way the existing story does it. |
chad1008
added
[Package] Components
…ithout a provided `value` prop Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
|
Size Change: +24 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
chad1008
force-pushed
the
enhance/ComboboxControl-add-uncontrolled-support
branch
from
7f094ff
to
0ff6dee
Compare
ciampo
approved these changes
Jul 28, 2022
There was a problem hiding this comment.
Code changes LGTM 🚀
Before:
Kapture.2022-07-28.at.10.28.38.mp4
After:
Kapture.2022-07-28.at.10.26.29.mp4
Also tested the component in when editing a page's parent in the editor.
While working on the above PR, @ciampo and I noticed that ComboboxControl doesn't currently work as an uncontrolled component
The power of unit tests!
That said, if others think it would be beneficial, I'm happy to implement a new story on this PR as well... I can just swap out the logged value for a printed output the way the existing story does it.
I don't think there's a need — we usually use controlled examples on Storybook
|
Thanks @ciampo! |
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.
What?
Related: #42403
Add the ability to implement
ComboboxControlas an uncontrolled componentWhy?
While working on the above PR, @ciampo and I noticed that
ComboboxControldoesn't currently work as an uncontrolled component. When novalueprop is defined, there's nothing to pass to theinputelement's value attribute, so it ends up being undefined.How?
The wizardry of the
useControlledValue()hook allows us to use a passed in value prop when the component is controlled, but fall back to managing this state internally in the absence of a prop.Testing Instructions
After applying this patch, add the following test snippet to the end of
packages/components/src/combobox-control/stories/index.js:This will add an Uncontrolled Test story in Storybook. Launch Storybook locally and unsure that story works as expected. In the console, confirm that the abbreviation for the selected country is logged when you select it (this confirms the
onChangefunction is being called).