-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove reset button in behaviors, use dropdown option instead #51464
Conversation
Size Change: -521 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
Flaky tests detected in b5bdc9e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5257153122
|
Thanks for the PR, as noted in #51357 (comment), I think this gets close. But when you select "Default" in the dropdown, its resting state should say Default, not Lightbox. So we shouldn't have smarts that suggest what is inherited, this needs to be solved separately. There's also #49278 to provide an inheritance interface. |
Done! |
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.
Looks good to me as long as the tests are fixed and/or rerun to pass 👍
We have to update them. As before, they used the selector to check that reading from theme.json was working as expected. |
|
||
// The behaviors dropdown should be present and the value should be set to | ||
// `lightbox`. | ||
await expect( select ).toHaveValue( 'lightbox' ); |
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.
This test now makes no sense, as the behavior will be always 'Default' if we don't interact with the selector.
8db1d0d
to
286541f
Compare
Closed as #51357 already has the 'Default' option implemented. |
What?
After a conversation in another draft PR regarding behaviors. I did a proof of concept of having a default option in the dropdown.
Conversation:
#51357 (comment)
Why?
A reset button is not a pattern we are using widely. Also, I still don't know how the interface will be with more than one behavior applied.
How?
Converted the reset button into a dropdown option.
Testing Instructions
Screenshots or screencast
Screen.Recording.2023-06-13.at.16.37.22.mov