-
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
Inserter: Use 40px default size for toggle button #68155
Conversation
Size Change: +10 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
Flaky tests detected in f599bdb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12423708264
|
e35bcf6
to
d95645e
Compare
@@ -60,6 +60,7 @@ const defaultRenderToggle = ( { | |||
|
|||
return ( | |||
<Wrapper | |||
__next40pxDefaultSize={ toggleProps.as ? undefined : true } |
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.
Consumers who are rendering a custom toggle button with the as
prop will need to take care of this themselves.
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.
Same question as #68157 (comment) re. checking if toggleProps.as ===
Button` ?
// TODO: Consider passing size="small" to the Inserter toggle instead. | ||
// Special dimensions for this button. | ||
min-width: $button-size-small; | ||
height: $button-size-small; |
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.
Virtually all usage in Gutenberg seem to be overriding the toggle Button size to be 24px. They should consider using size="small"
instead of doing this with custom CSS (out of scope for this PR, so I left TODO comments).
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 like a worthy follow-up 👍
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. |
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 expected, style overrides prevent the code changes from causing any visual differences. Agreed that we should consider using size="small"
instead, that's a good small follow-up task.
🚀
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.
Tested all toggle buttons and verified they're unchanged 👍
// TODO: Consider passing size="small" to the Inserter toggle instead. | ||
// Special dimensions for this button. | ||
min-width: $button-size-small; | ||
height: $button-size-small; |
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 like a worthy follow-up 👍
In preparation for #65751
What?
Ensures/verifies that the
Inserter
toggle buttons won't break when the default Button size becomes 40px.Testing Instructions
Smoke test some Inserter usages in the app. There should be no visual changes.
Screenshots or screencast
The black plus button is the Inserter toggle button.