-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix: Button Replace remaining 40px default size violations [Block Editor 5] #65361
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @parthVataliya16. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @PARTHVATALIYA! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
packages/block-editor/src/components/inspector-controls-tabs/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inspector-popover-header/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inspector-popover-header/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/list-view/block-select-button.js
Outdated
Show resolved
Hide resolved
@@ -51,8 +51,7 @@ const InsertFromURLPopover = ( { | |||
value={ src } | |||
/> | |||
<Button | |||
// TODO: Switch to `true` (40px size) if possible | |||
__next40pxDefaultSize={ false } | |||
__next40pxDefaultSize |
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.
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 think so. After #65158 lands, this media placeholder will also need to be updated to be a small button in the suffix slot, like in #65158 (comment).
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.
#65158 has landed, so after merging latest trunk into this branch, we can move the button to the suffix slot.
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.
I took another look and can confirm that URLPopover
doesn't have a suffix
— we can simply set __next40pxDefaultSize
to true
(like originally done by @PARTHVATALIYA ). The input will automatically match the new height, as per the screenshot above.
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.
Actually we want to consolidate the UI pattern to have buttons in the suffix (see #64709 (comment)). And I noticed that this is a big enough change to warrant a separate code review, so I submitted a separate PR to deal with it (#65656).
In any case, not a blocker for this PR.
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.
Thanks for the ping. Link Control looks like it might need some tweaks.
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.
Thanks a bunch for working on this one @PARTHVATALIYA 🙌
Took this for a quick run, and I agree with @ciampo that there are a bunch of related style overrides that can be cleaned up.
The block inspector tabs with icons can be an exception IMHO, as they're a trickier scenario. For that one, maybe it makes sense to add a comment as to why the size prop doesn't apply.
Hey @PARTHVATALIYA , just making sure you've seen Lena's comment — we need to merge upstream |
@PARTHVATALIYA I left more comments, let me know if you are able to address them or if you have any questions. Thank you! |
@ciampo I added all the changes as your suggestion. Please let me know if I have miss something or need to do. Thank you! |
packages/block-editor/src/components/inspector-popover-header/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inspector-popover-header/index.js
Outdated
Show resolved
Hide resolved
@@ -51,8 +51,7 @@ const InsertFromURLPopover = ( { | |||
value={ src } | |||
/> | |||
<Button | |||
// TODO: Switch to `true` (40px size) if possible | |||
__next40pxDefaultSize={ false } | |||
__next40pxDefaultSize |
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 took another look and can confirm that URLPopover
doesn't have a suffix
— we can simply set __next40pxDefaultSize
to true
(like originally done by @PARTHVATALIYA ). The input will automatically match the new height, as per the screenshot above.
Thank you! There are three remaining comments to address (#65361 (comment), #65361 (comment), #65361 (comment)) and then we'll be able to merge ⭐ |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
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.
LGTM 🚀
Thank for the collaboration, @PARTHVATALIYA 🙇
…tor 5] (#65361) * Fix: inspector control tabs to use 40px default button size * Fix: inspector popover header to use 40px default button size * Fix: link control to use 40px default button size * Fix: list view to use 40px default button size * Fix: media placeholder to use 40px default button size * Add size attribute to inspector popover header and list view * Revert the changes for submit button * Replace button component * Remove override CSS * Remove __next40pxDefaultSize prop Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> * Remove __next40pxDefaultSize prop Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> --------- Co-authored-by: PARTHVATALIYA <parthvataliya@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
Part of - #65018
What?
Issue - #65018, To use default to 40px for the button.
Why?
To make the consistent button across Gutenberg.
How?
Change from
__next40pxDefaultSize={ false }
to__next40pxDefaultSize
on component.Testing Instructions