-
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
Button: Replace ButtonGroup usage with ToggleGroupControl #65346
Conversation
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. |
Size Change: -21 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
return ( | ||
<PanelBody title={ __( 'Settings' ) }> | ||
<ButtonGroup aria-label={ __( 'Button width' ) }> | ||
<ToggleGroupControl |
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.
I feel like we should add the isBlock
prop here:
- it will make the control full-width
- it will show a border
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.
Makes sense. I'll add the prop.
What's the reason for having border style connected to the isBlock
prop?
Update: If you use isBlock
and isDeselectable
combo, the border isn't applied. This looks like a bug.
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.
I believe it was decided together with design folks given the specific constraints that we faced on the component — but I'll let Lena clarity if necessary.
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.
Yes, the relationship between the props and the patterns are not very clear at the moment. The three "valid" patterns we can have are:
- Text options +
isBlock
- Icon options
- Icon options +
isDeselectable
I'll make a note about this for when we stabilize this component (#61067).
onChange={ ( newWidth ) => | ||
setAttributes( { width: newWidth } ) | ||
} | ||
isDeselectable |
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.
@t-hamano suggested in #52223 (comment) that we add an "Auto" or "None" option for the reset. I think that might be better than the isDeselectable
variant in this case, because our options items here are not icon buttons.
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.
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.
Mmm, good point about the "None" discussion.
The thing is, "Text options + isDeselectable
" is not a valid pattern for this component 😕 For the sake of this PR, could we do a "None" option, or maybe add a separate Clear or Reset button?
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.
To be honest, the reason I suggested adding Auto/None options here was because I wasn't aware of the isDeselectable
prop 😅
I also learned from this discussion that "Text option + isDeselectable
" is not a valid pattern.
When an explicit percentage width is not set on a button, the button's width grows and shrinks depending on the text inside, so "Auto" feels more appropriate than "None".
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.
Sounds good! I guess we should go with a adding a "Auto" option?
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.
"Auto" should work fine, but as mentioned in this comment, it may cause unintended layouts when localized.
If there was a custom width (UnitControl) UI, we could set the value to empty (undefined
) via that, but currently there isn't.
Given this, would it be a good idea to add a reset button?
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.
As explained in #65340 (comment), if we currently added a "Reset" button, we'd still face the issue that ToggleGroupControl
selects a value when it receives focus.
So, with the current behaviour of the component, an Auto
option is the only viable option IMO. We could otherwise pause the work here until ToggleGroupControl
gets a new design and behaviour.
This could possibly be addressed in a follow-up, but #65378 has been filed suggesting adding a label. |
FWIW, the control already has an accessibility label, "Button width", but a visible label definitely makes sense IMHO. Right now it's not clear whether this is about width, height, or anything else in particular. |
Agreed — we could add a visible label also in this PR, since it should be as easy as remove the related prop to hide it visually. |
5056c09
to
635dca8
Compare
I've updated the code to show the label; see the update screenshot in the PR description. As I can see, the only "blocker" here is the value deselection option. This can probably be handled separately, as this migration PR doesn't change the Button setting behavior. |
Flaky tests detected in 635dca8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11013604125
|
I'll let @mirka chime in here, since I'm not sure that the current way |
The focus bug (#62981) has been fixed, so for the deselection functionality I think we can either replicate the implementation in #65340 (an enhancement so users can now input a custom width), or just add a separate reset button. Enhancement could be preferable, since there's already a feature request (#62247). |
I would like to address #62247 separately and then return to this PR. |
635dca8
to
df9c37f
Compare
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.
P.S. We will need to update E2E tests. |
I just pushed the fix for e2e tests. |
…65346) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: andreawetzel <awetz583@git.wordpress.org>
…65346) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: andreawetzel <awetz583@git.wordpress.org>
@Mamaduka Do you mind following up with changing the label to "Width" instead of "Button width"? It's already clear this is the button's width. |
What?
Fixes #65378.
PR replaces the
ButtonGroup
component withToggleGroupControl
in the Button block controls.Why?
Part of #65339.
Testing Instructions
Testing Instructions for Keyboard
Same.
Screenshot