Fix: Button Replace remaining 40px default size violations [Block Editor 1]. #65034

vipul0425 · 2024-09-04T07:00:37Z

Part of - #65018

What?

This PR adds the 40px default size to invalid block warning, multiple block usage warning, block compare, block breadcrumbs and block mover.

Why?

To make the consistent button across Gutenberg.

How?

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

Testing steps and screenshots are added below.

…umbs.

github-actions · 2024-09-04T07:00:50Z

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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: vipul0425 <vipulgupta003@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>

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

vipul0425 · 2024-09-04T07:04:54Z

packages/block-editor/src/components/block-compare/block-view.js

@@ -31,8 +31,7 @@ export default function BlockView( {

 <div className="block-editor-block-compare__action">
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


This can be seen when any block as a validation error and you click on resolve.

Before After

vipul0425 · 2024-09-04T07:07:10Z

packages/block-editor/src/components/block-edit/multiple-usage-warning.js

@@ -24,17 +24,15 @@ export function MultipleUsageWarning( {
 <Warning
 actions={ [
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


This occurs when a block has multiple supports set to false in block.json and you add that block multiple times in the editor.

Before After

vipul0425 · 2024-09-04T07:11:35Z

packages/block-editor/src/components/block-list/block-invalid-warning.js

@@ -101,8 +101,7 @@ export default function BlockInvalidWarning( { clientId } ) {
 <Warning
 actions={ [
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


The invalid warning can be seen when a block's content is invalid after saving the page and then returning to it.

Before After

vipul0425 · 2024-09-04T07:14:31Z

Hi @mirka , for the block breadcrumbs and block mover, I have set __next40pxDefaultSize to true, but the custom styles defined in those blocks are overriding it to 20px. Should we update those custom styles as well?

ciampo · 2024-09-04T15:09:56Z

Hi @mirka , for the block breadcrumbs and block mover, I have set __next40pxDefaultSize to true, but the custom styles defined in those blocks are overriding it to 20px. Should we update those custom styles as well?

Mhh, good question!

Overriding the height shouldn't happen in the first place.

For the block breadcrumbs, we could consider setting size="small", which would change the height to 24px — although I'm aware that screen real estate is precious and we may not want to increase their height.

In both cases, an alternative could also be to use vanilla <button /> elements in combination with <Icon > and <Tooltip /> where needed, and do extra work to make them accessibleWhenDisabled.

We should also consider that all <Button />s in the block toolbar also have their height overridden to 48px anyway.

I'm not sure if neither of those approaches are better than the CSS height override, though — I'd love to hear @WordPress/gutenberg-components and @WordPress/gutenberg-design 's thoughts.

jameskoster · 2024-09-04T15:26:19Z

On trunk the breadcrumb buttons seem to have a style override to make them 24px:

Removing that and using the small size makes sense to me.

For the movers; there appears to be space to make them 24px with a couple of adjustments to maintain visual appearance. Again, could we use the small size and remove the override?

ciampo · 2024-09-04T15:56:26Z

@vipul0425 , could try to set size="small" on both instances and remove the height style override?

vipul0425 · 2024-09-05T06:01:16Z

Hi @ciampo For the breadcrumbs it is working fine.

However, when I apply that to the block mover buttons and remove the height-specific styles from its stylesheet, the height set by the toolbar component has higher specificity and takes priority. Should we override this in the block-mover button styles using $button-size-small, or is there another approach you'd recommend?

jasmussen · 2024-09-05T06:36:25Z

Thanks for working on this!

tyxla

Thanks for helping with the button sizing @vipul0425 🙌

Note that some snapshot tests will be failing as a consequence, and you might have to run npm run test:unit:update packages/block-editor/src/components/ to update them and then commit them as part of the PR.

ciampo · 2024-09-05T14:46:39Z

packages/block-editor/src/components/block-breadcrumb/style.scss

@@ -23,7 +23,6 @@
 }

 .block-editor-block-breadcrumb__button.components-button {
- height: $button-size-small;


We can potentially remove a lot more style overrides (and the .components-button classname, which should not be used at all). It also looks like the tertiary variant is not really needed.

Something like this (click to show diff)

diff --git a/packages/block-editor/src/components/block-breadcrumb/index.js b/packages/block-editor/src/components/block-breadcrumb/index.js index 865d8d0600..be0ab9fc45 100644 --- a/packages/block-editor/src/components/block-breadcrumb/index.js +++ b/packages/block-editor/src/components/block-breadcrumb/index.js @@ -68,7 +68,6 @@ function BlockBreadcrumb( { rootLabelText } ) { <Button size="small" className="block-editor-block-breadcrumb__button" - variant="tertiary" onClick={ () => { // Find the block editor wrapper for the selected block const blockEditor = blockRef.current?.closest( diff --git a/packages/block-editor/src/components/block-breadcrumb/style.scss b/packages/block-editor/src/components/block-breadcrumb/style.scss index a1ae3bf2f1..af56971fe4 100644 --- a/packages/block-editor/src/components/block-breadcrumb/style.scss +++ b/packages/block-editor/src/components/block-breadcrumb/style.scss @@ -22,39 +22,11 @@ } } -.block-editor-block-breadcrumb__button.components-button { - line-height: $button-size-small; - padding: 0; - position: relative; - - &:hover:not(:disabled) { - text-decoration: underline; - box-shadow: none; - } - - &:focus { - box-shadow: none; - } - - &:focus::before { - content: ""; - display: block; - position: absolute; - border-radius: $radius-block-ui; - top: $border-width; - right: $border-width; - bottom: $border-width; - left: $border-width; - - @include button-style__focus(); - } -} - .block-editor-block-breadcrumb__current { cursor: default; } -.block-editor-block-breadcrumb__button.components-button, +.block-editor-block-breadcrumb__button.block-editor-block-breadcrumb__button, .block-editor-block-breadcrumb__current { color: $gray-900; padding: 0 $grid-unit-10;

Kapture.2024-09-05.at.16.45.51.mp4

What do you folks think? cc @WordPress/gutenberg-design

ciampo · 2024-09-05T14:53:15Z

Hi @ciampo For the breadcrumbs it is working fine.

Cool! I actually looked into it and left a comment since I think we have an opportunity to remove almost every style override there.

However, when I apply that to the block mover buttons and remove the height-specific styles from its stylesheet, the height set by the toolbar component has higher specificity and takes priority. Should we override this in the block-mover button styles using $button-size-small, or is there another approach you'd recommend?

Good point. To be honest, the block mover buttons have a lot of custom styles (which, in their turn, override the custom button styles from the toolbar). I'm tempted to leave the style overrides untouched for the scope of this PR, and otherwise consider switching to a vanilla HTML <button /> as an alternative for a low-priority follow-up PR.

…e-65018

ciampo

Looking good! Thank you for the cleanup work 🙌

vipul0425 added 4 commits September 4, 2024 12:02

feat: Adds 40px height in multiple usage warning.

7d7a48f

feat: Adds 4px size in invalid warining.

5a0176f

feat: Adds the 40px size in block compare component.

77cdf42

feat: Makes the __next40pxDefaultSize true in block mover and breadcr…

f31118c

…umbs.

vipul0425 requested a review from ellatrix as a code owner September 4, 2024 07:00

fix: block compare default size.

ff95c61

vipul0425 commented Sep 4, 2024

View reviewed changes

fix: Invlid warning button size.

607eba2

vipul0425 commented Sep 4, 2024

View reviewed changes

vipul0425 mentioned this pull request Sep 4, 2024

Button: Replace remaining 40px default size violations #65018

Open

21 tasks

tyxla requested a review from a team September 4, 2024 11:59

ciampo assigned vipul0425 Sep 4, 2024

ciampo added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Package] Block editor /packages/block-editor labels Sep 4, 2024

fix: Adds size prop to breadcrumb buttons.

bf1f272

tyxla reviewed Sep 5, 2024

View reviewed changes

tyxla added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Enhancement A suggestion for improvement. labels Sep 5, 2024

ciampo reviewed Sep 5, 2024

View reviewed changes

fix: Updates the snapshot.

733e3ce

vipul0425 added 2 commits September 6, 2024 11:02

refactor: Removes extra button [custom styles.

0a6ca4f

Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/issu…

8fa351c

…e-65018

vipul0425 requested review from ciampo and tyxla September 6, 2024 08:40

ciampo approved these changes Sep 6, 2024

View reviewed changes

ciampo merged commit 0fc06de into WordPress:trunk Sep 6, 2024
62 checks passed

github-actions bot added this to the Gutenberg 19.3 milestone Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Button Replace remaining 40px default size violations [Block Editor 1]. #65034

Fix: Button Replace remaining 40px default size violations [Block Editor 1]. #65034

vipul0425 commented Sep 4, 2024

github-actions bot commented Sep 4, 2024 •

edited

Loading

vipul0425 Sep 4, 2024

vipul0425 Sep 4, 2024

vipul0425 Sep 4, 2024

vipul0425 commented Sep 4, 2024 •

edited

Loading

ciampo commented Sep 4, 2024

jameskoster commented Sep 4, 2024

ciampo commented Sep 4, 2024

vipul0425 commented Sep 5, 2024 •

edited

Loading

jasmussen commented Sep 5, 2024

tyxla left a comment

ciampo Sep 5, 2024 •

edited

Loading

jameskoster Sep 5, 2024

ciampo commented Sep 5, 2024

ciampo left a comment

Fix: Button Replace remaining 40px default size violations [Block Editor 1]. #65034

Fix: Button Replace remaining 40px default size violations [Block Editor 1]. #65034

Conversation

vipul0425 commented Sep 4, 2024

What?

Why?

How?

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

vipul0425 Sep 4, 2024

Choose a reason for hiding this comment

vipul0425 Sep 4, 2024

Choose a reason for hiding this comment

vipul0425 Sep 4, 2024

Choose a reason for hiding this comment

vipul0425 commented Sep 4, 2024 • edited Loading

ciampo commented Sep 4, 2024

jameskoster commented Sep 4, 2024

ciampo commented Sep 4, 2024

vipul0425 commented Sep 5, 2024 • edited Loading

jasmussen commented Sep 5, 2024

tyxla left a comment

Choose a reason for hiding this comment

ciampo Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

jameskoster Sep 5, 2024

Choose a reason for hiding this comment

ciampo commented Sep 5, 2024

ciampo left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 4, 2024 •

edited

Loading

vipul0425 commented Sep 4, 2024 •

edited

Loading

vipul0425 commented Sep 5, 2024 •

edited

Loading

ciampo Sep 5, 2024 •

edited

Loading