ComboboxControl: Display reset button only if there's a value #72595
Conversation
juanfra
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 1657aa3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18801032211
|
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Nice enhancement, thanks!
-
I just noticed the Storybook has
allowReset: falseset, which is strange because it isn't the default setting. Let's take this opportunity to remove that. -
If it isn't too much extra work, can we also hide the button in this case too, where there is technically a value set but it isn't visible? (Not a blocker to merging though — the changes in this PR are already an improvement over trunk.)
CleanShot.2025-10-24.at.16-46-43.mp4
| it( 'should render with Reset button disabled', () => { | ||
| render( | ||
| <Component | ||
| options={ timezones } | ||
| label={ defaultLabelText } | ||
| value={ timezones[ 0 ].value } |
There was a problem hiding this comment.
Is this test outdated now? The reset button is never going to be disabled.
| <Component | ||
| options={ timezones } | ||
| label={ defaultLabelText } | ||
| value={ targetOption.value } |
There was a problem hiding this comment.
Setting a value prop like this is not going to be very realistic in the tests for uncontrolled mode, because the static value will basically force an internal value even after a reset.
It would be better not set a value from the prop, but instead simulate the user picking a value (which we're already doing in this test here).
There was a problem hiding this comment.
Perfect, will update this one.
| await user.click( resetButton ); | ||
|
|
||
| expect( input ).toHaveValue( '' ); | ||
| expect( resetButton ).toBeDisabled(); |
There was a problem hiding this comment.
Can we still test for not.toBeInTheDocument?
…e because the combobox is expanded.
|
Thank you for the review @mirka :) I’ve addressed the feedback. |
What?
Closes #72522
We're updating the ComboboxControl, so that it shows the reset button only if there's a value. The reset button should only appear when there's an actual value to clear, and be hidden when the combobox is empty.
Why?
To improve UX. Removing unnecessary and confusing UI elements when there's nothing to clear.
Testing Instructions
nvm use && npm run storybook:dev?path=/docs/components-comboboxcontrol--docsScreenshots or screencast
combobox-reset-with-value.mp4