-
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
ToggleGroupControl: Update large button size to 32px #57338
Conversation
const normalizedSize = | ||
__next40pxDefaultSize && size === 'default' ? '__unstable-large' : size; |
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.
We usually don't do this kind of preemptive size normalization in other components, but in this specific case it was a lot cleaner this way. Otherwise we'd have to drill the prop down to multiple subcomponents, touching a handful of more files.
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.
Agreed. We should be able to clean this up after the migration is over and we remove temporary props and size
values
const styles = { | ||
default: css` | ||
min-height: 36px; | ||
padding: 2px; | ||
`, | ||
'__unstable-large': css` | ||
min-height: 40px; | ||
padding: 3px; | ||
`, |
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 am tempted to simplify this down to a
if ( size === 'default` ) {
return css`/* ... */`;
}
return css`/* ... */`;
but I'm going to keep this size variant object around because I think we'll be asked to make a 32px compact size variant soon.
Flaky tests detected in a803192. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7321454753
|
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.
🚀
Tests well as per instructions
packages/components/src/toggle-group-control/toggle-group-control-option-base/styles.ts
Outdated
Show resolved
Hide resolved
const normalizedSize = | ||
__next40pxDefaultSize && size === 'default' ? '__unstable-large' : size; |
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.
Agreed. We should be able to clean this up after the migration is over and we remove temporary props and size
values
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Closes #45532
What?
Updates the size of the ToggleGroupControl buttons in the large size variant to be 32px (used to be 34px).
Also fixes a bug where icon buttons did not have square proportions when
size = 'default'
&&__next40pxDefaultSize = true
.Why?
As requested, to be more cohesive with the overall sizing scheme.
Testing Instructions
In the Storybook stories for ToggleGroupControl, see that the button sizes for
size = '__unstable-large'
and__next40pxDefaultSize = true
are now 32px. Check both the text label buttons and icon buttons.The overall footprint (height) including the border around the entire button group should remain the same at 40px.
Screenshots or screencast
Bug fix for icon buttons where
size = 'default'
&&__next40pxDefaultSize = true
: