Fix: Button Replace remaining 40px default size violations [Block Editor 2] by dhruvang21 · Pull Request #65308 · WordPress/gutenberg

dhruvang21 · 2024-09-13T06:50:40Z

Part of - #65018

What?

Issue - #65018, To use default to 40px for the button.

Why?

To make the consistent button across Gutenberg, and we would have a lint rule added once fixed, all the button usage.

How?

Change from __next40pxDefaultSize={ false } to __next40pxDefaultSize on component.

Testing Instructions

Screenshot is added for individual changed files.

github-actions · 2024-09-13T06:50:55Z

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: @Dhruvang2001.

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: Dhruvang2001.

Co-authored-by: dhruvang21 <dhruvang21@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: hbhalodia <hbhalodia@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 · 2024-09-13T06:51:01Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @dhruvang21! 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.

hbhalodia · 2024-09-13T07:01:52Z

Hi @dhruvang21, Can you please add before/after screenshots for the changes?

This would help us to check the change visually.

Thank You,

dhruvang21 · 2024-09-13T07:25:23Z

 						<Button
-							// TODO: Switch to `true` (40px size) if possible
-							__next40pxDefaultSize={ false }
+							__next40pxDefaultSize


Before
After

These buttons currently have style overrides that are unnecessary and can be replaced with the compact size

Let's remove the __next40pxDefaultSize on all 4 buttons in this file and add `size="compact"

Let's also remove the size="default" prop on the last button (replaced by size="compact")

We can, therefore, remove these overrides, which are now unnecessary

Finally, as an extra step, I noticed that there are more style overrides for when the "show icon labels" preference is enabled. Those style overrides use the internal components-button classname, which is private. We can make the following changes to improve the code:

add className="block-editor-patterns__grid-pagination-button" to all 4 instances of Button in the file

tweak the CSS selector in the style overrides:

diff --git a/packages/block-editor/src/components/block-patterns-paging/style.scss b/packages/block-editor/src/components/block-patterns-paging/style.scss index f5f34d8212..b903334460 100644 --- a/packages/block-editor/src/components/block-patterns-paging/style.scss +++ b/packages/block-editor/src/components/block-patterns-paging/style.scss @@ -23,8 +23,7 @@ } .show-icon-labels { - .block-editor-patterns__grid-pagination { - .components-button { + .block-editor-patterns__grid-pagination-button { width: auto; // Hide the button icons when labels are set to display... span {

ciampo

Thank you @dhruvang21 for working on this, and sorry for the delay in reviewing your PR.

For future work, it would greatly help if you could add screenshots (or a comment to why to you couldn't produce a screenshot).

I recommend you rebase (or merge trunk into) this PR, so that it's up to date with the upstream repository.

I also left some comments that would need to be addressed before we can move forward.

Thank you again, and let me know in case anything isn't clear.

ciampo · 2024-09-24T10:44:33Z

 		<Button
-			// TODO: Switch to `true` (40px size) if possible
-			__next40pxDefaultSize={ false }
+			__next40pxDefaultSize


Noting that this Button is used in a deprecated component, and therefore we should be ok to make this change without the need to really test much about it.

ciampo · 2024-09-24T11:05:23Z

 		<Button
-			// TODO: Switch to `true` (40px size) if possible
-			__next40pxDefaultSize={ false }
+			__next40pxDefaultSize


This component also doesn't seem to be used anymore — however, I managed to find a screenshot of that the UI would look like when rendered in the browser:

Judging from the screenshot, I think that we're good with the changes to this button

ciampo · 2024-09-24T12:01:28Z

 						<Button
-							// TODO: Switch to `true` (40px size) if possible
-							__next40pxDefaultSize={ false }
+							__next40pxDefaultSize


These buttons currently have style overrides that are unnecessary and can be replaced with the compact size

Let's remove the __next40pxDefaultSize on all 4 buttons in this file and add `size="compact"

Let's also remove the size="default" prop on the last button (replaced by size="compact")

We can, therefore, remove these overrides, which are now unnecessary

Finally, as an extra step, I noticed that there are more style overrides for when the "show icon labels" preference is enabled. Those style overrides use the internal components-button classname, which is private. We can make the following changes to improve the code:

add className="block-editor-patterns__grid-pagination-button" to all 4 instances of Button in the file

tweak the CSS selector in the style overrides:

diff --git a/packages/block-editor/src/components/block-patterns-paging/style.scss b/packages/block-editor/src/components/block-patterns-paging/style.scss index f5f34d8212..b903334460 100644 --- a/packages/block-editor/src/components/block-patterns-paging/style.scss +++ b/packages/block-editor/src/components/block-patterns-paging/style.scss @@ -23,8 +23,7 @@ } .show-icon-labels { - .block-editor-patterns__grid-pagination { - .components-button { + .block-editor-patterns__grid-pagination-button { width: auto; // Hide the button icons when labels are set to display... span {

ciampo · 2024-09-24T12:28:20Z

 		<Button
-			// TODO: Switch to `true` (40px size) if possible
-			__next40pxDefaultSize={ false }
+			__next40pxDefaultSize


Looking good

ciampo · 2024-09-24T12:39:09Z

This file was removed in #65204 (comment), and therefore these changes are not necessary anymore.

In any case, they would have not caused any meaningful differences because those buttons' styles were heavily overridden 😓

dhruvang21 · 2024-09-24T13:58:39Z

ciampo

Hi @ciampo,

Thanks for adding the screenshots! To be honest, I tried to locate the buttons but wasn't successful. I’ll make the necessary changes as per your instructions in the next commit.

Thanks again!

ciampo · 2024-09-25T17:18:28Z

These style overrides can be also removed

ciampo · 2024-09-25T17:20:14Z

@Dhruvang2001 I believe that this is the last comment left to address, before being able to merge. Almost there!

ciampo

Excellent! Thank you again for the collaboration, @Dhruvang2001 🙏

dhruvang21 · 2024-09-26T09:31:25Z

Excellent! Thank you again for the collaboration, @Dhruvang2001 🙏

You're very welcome! It's been great collaborating with you as well. Looking forward to more in the future! 🙌

dhruvang21 requested a review from ellatrix as a code owner September 13, 2024 06:50

github-actions Bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 13, 2024

dhruvang21 mentioned this pull request Sep 13, 2024

Button: Replace remaining 40px default size violations #65018

Closed

21 tasks

dhruvang21 commented Sep 13, 2024

View reviewed changes

mirka added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 13, 2024

mirka assigned dhruvang21 Sep 13, 2024

mirka requested a review from a team September 13, 2024 09:58

ciampo reviewed Sep 24, 2024

View reviewed changes

Add __next40pxDefaultSize for block-editor 2 files

0dde589

dhruvang21 force-pushed the bugfix/issue-65018 branch from 7157ce5 to 0dde589 Compare September 25, 2024 06:09

Add size compact to buttons

3cd42d6

ciampo reviewed Sep 25, 2024

View reviewed changes

Remove style overrides

6a0be67

ciampo approved these changes Sep 26, 2024

View reviewed changes

ciampo merged commit d0bcf79 into WordPress:trunk Sep 26, 2024

github-actions Bot added this to the Gutenberg 19.4 milestone Sep 26, 2024

mirka added the Backport to WP 6.7 Beta/RC label Oct 1, 2024

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 labels Oct 6, 2024

Conversation

dhruvang21 commented Sep 13, 2024

What?

Why?

How?

Testing Instructions

Uh oh!

github-actions Bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unlinked Accounts

Uh oh!

github-actions Bot commented Sep 13, 2024

Uh oh!

hbhalodia commented Sep 13, 2024

Uh oh!

dhruvang21 Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciampo Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

ciampo Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

ciampo Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ciampo Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciampo Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

ciampo Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

dhruvang21 commented Sep 24, 2024

Uh oh!

ciampo Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

ciampo commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

dhruvang21 commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions Bot commented Sep 13, 2024 •

edited

Loading

dhruvang21 Sep 13, 2024 •

edited

Loading

ciampo Sep 24, 2024 •

edited

Loading

ciampo Sep 24, 2024 •

edited

Loading

ciampo commented Sep 25, 2024 •

edited

Loading