Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: Button Replace remaining 40px default size violations [Block Editor 5] #65361

Merged
merged 13 commits into from
Sep 25, 2024
Merged

Fix: Button Replace remaining 40px default size violations [Block Editor 5] #65361

merged 13 commits into from
Sep 25, 2024

Conversation

PARTHVATALIYA
Copy link
Contributor

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

  • Testing steps and screenshots are added below.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 16, 2024 •
edited
Loading

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 props-bot label.

Unlinked Accounts

The 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.

Unlinked contributors: parthVataliya16.

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>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions GitHub Actions
Copy link

👋 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.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 16, 2024
@PARTHVATALIYA PARTHVATALIYA mentioned this pull request Sep 16, 2024
Closed
21 tasks
PARTHVATALIYA
PARTHVATALIYA commented Sep 16, 2024
View reviewed changes
packages/block-editor/src/components/media-placeholder/index.js Outdated
@@ -51,8 +51,7 @@ const InsertFromURLPopover = ( {
value={ src }
/>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before:
media-placeholder-1-before

After:
media-placeholder-1-after

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirka will this be also affected by #65158 ?

Copy link
Member

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).

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirka I noticed that #65158 deals with URLInput, while this file is dealing with URLPopover, which doesn't seem to have a suffix. Should we just add the __next40pxDefaultSize prop to Button and use CSS to update the input height to 40px too?

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mirka , I didn't initially understand that you were suggesting to use an InputControl. The work is going to be carried out in #65656, which means that there's nothing left to be done in this PR 🎉

@PARTHVATALIYA PARTHVATALIYA changed the title Fix: inspector control tabs to use 40px default button size Fix: Button Replace remaining 40px default size violations [Block Editor 5] Sep 16, 2024
getdave
getdave reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@getdave getdave left a 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.

packages/block-editor/src/components/link-control/index.js Outdated Show resolved Hide resolved
packages/block-editor/src/components/link-control/settings-drawer.js Show resolved Hide resolved
packages/block-editor/src/components/link-control/index.js Outdated Show resolved Hide resolved
@mirka mirka requested a review from a team September 16, 2024 11:03
@mirka mirka added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 16, 2024
tyxla
tyxla reviewed Sep 17, 2024
View reviewed changes
Copy link
Member

@tyxla tyxla left a 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.

@ciampo
Copy link
Contributor

ciampo commented Sep 24, 2024

Hey @PARTHVATALIYA , just making sure you've seen Lena's comment — we need to merge upstream trunk back into this PR in order to move forward. Let us know if you need help with that.

@ciampo
Copy link
Contributor

ciampo commented Sep 24, 2024

@PARTHVATALIYA I left more comments, let me know if you are able to address them or if you have any questions. Thank you!

@PARTHVATALIYA
Copy link
Contributor Author

PARTHVATALIYA commented Sep 25, 2024 •
edited
Loading

@ciampo I added all the changes as your suggestion. Please let me know if I have miss something or need to do.

Thank you!

ciampo
ciampo reviewed Sep 25, 2024
View reviewed changes
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/media-placeholder/index.js Outdated
@@ -51,8 +51,7 @@ const InsertFromURLPopover = ( {
value={ src }
/>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor

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.

@ciampo
Copy link
Contributor

ciampo commented Sep 25, 2024 •
edited
Loading

Please let me know if I have miss something or need to do.

Thank you! There are three remaining comments to address (#65361 (comment), #65361 (comment), #65361 (comment)) and then we'll be able to merge ⭐

PARTHVATALIYA and others added 2 commits September 25, 2024 14:16
@PARTHVATALIYA @ciampo
Remove __next40pxDefaultSize prop
e287a6d 
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
@PARTHVATALIYA @ciampo
Remove __next40pxDefaultSize prop
5086a1d 
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
ciampo
ciampo approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a 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 🙇

@ciampo ciampo merged commit e321b5a into WordPress:trunk Sep 25, 2024
62 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 25, 2024
This was referenced Sep 27, 2024
Merged
Closed
@mirka mirka added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 1, 2024
@noisysocks noisysocks added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 6, 2024
noisysocks pushed a commit that referenced this pull request Oct 6, 2024
@PARTHVATALIYA @getdave
@mirka @ciampo @tyxla @noisysocks
Fix: Button Replace remaining 40px default size violations [Block Edi…
4f835b3 
…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>
@noisysocks noisysocks mentioned this pull request Oct 6, 2024
Closed
@draganescu draganescu mentioned this pull request Oct 15, 2024
Merged
@sirreal sirreal mentioned this pull request Oct 25, 2024
Closed
@t-hamano t-hamano mentioned this pull request Oct 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@getdave getdave getdave left review comments

@mirka mirka mirka left review comments

@tyxla tyxla tyxla left review comments

@ciampo ciampo ciampo approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix ellatrix is a code owner

Assignees

@PARTHVATALIYA PARTHVATALIYA

Labels
Backported to WP Core Pull request that has been successfully merged into WP Core First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 19.4
Development

Successfully merging this pull request may close these issues.
7 participants
@PARTHVATALIYA @ciampo @getdave @mirka @tyxla @noisysocks @parthVataliya16