Fix: Button Replace remaining 40px default size violations [Block Editor 3] #65225

vipul0425 · 2024-09-11T05:29:38Z

Part of - #65018

What?

This would fix in that in subtask block-editor-3.

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

Testing steps and screenshots are added below.

…fault size.

github-actions · 2024-09-11T05:29:52Z

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: DaniGuardiola <daniguardiola@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: ciampo <mciampini@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-11T05:32:51Z

packages/block-editor/src/components/block-variation-picker/index.js

@@ -64,8 +64,7 @@ function BlockVariationPicker( {
 { allowSkip && (
 <div className="block-editor-block-variation-picker__skip">
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


__next40pxDefaultSize has no effect here because the variation is set to link for this button, which overrides the size with height: auto.

vipul0425 · 2024-09-11T05:35:09Z

packages/block-editor/src/components/block-variation-transforms/index.js

@@ -35,8 +35,7 @@ function VariationsButtons( {
 </VisuallyHidden>
 { variations.map( ( variation ) => (
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before After

I'm not sure about this one. I think it should be more like ToggleGroupControl. IE the outer container is 40px, while the inner buttons are 32px (compact size).

@jameskoster what about just using a ToggleGroupControl?

The visible stroke in the resting state would create additional weight that is probably unwanted in this UI.

We have an issue to explore the ToggleGroupControl design here, which might make it appropriate for use in its vanilla state if implemented.

But until then the simpler approach is probably to just make these buttons compact (32px).

I have updated it to use compact size.

vipul0425 · 2024-09-11T05:36:18Z

packages/block-editor/src/components/button-block-appender/index.js

@@ -60,8 +60,7 @@ function ButtonBlockAppender(

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


Before After

The + icon looks too small. flex-shrink: 0 should fix but there might be a better way?

Related, the empty placeholder that appears adjacent in a Row/Stack has a 46px min-height which causes a mis-match:

It might be good to update that here.

Hi @jameskoster, I have removed the styles that were overriding the component button padding to fix the small icons. Also fixed the empty placeholder height.

vipul0425 · 2024-09-11T05:38:20Z

packages/block-editor/src/components/colors-gradients/dropdown.js

- __next40pxDefaultSize={ false }
- { ...toggleProps }
- >
+ <Button __next40pxDefaultSize { ...toggleProps }>


It was already taking 40px in height before, so there's no change after setting it to true.

vipul0425 · 2024-09-11T05:39:33Z

packages/block-editor/src/components/global-styles/color-panel.js

- __next40pxDefaultSize={ false }
- { ...toggleProps }
- >
+ <Button __next40pxDefaultSize { ...toggleProps }>


It was already taking 40px in height before, so there's no change after setting it to true.

vipul0425 · 2024-09-11T05:40:36Z

packages/block-editor/src/components/global-styles/filters-panel.js

@@ -190,8 +190,7 @@ export default function FiltersPanel( {
 return (
 <ItemGroup isBordered isSeparated>
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before After

vipul0425 · 2024-09-11T05:45:00Z

packages/block-editor/src/components/global-styles/shadow-panel-components.js

- __next40pxDefaultSize={ false }
- { ...toggleProps }
- >
+ <Button __next40pxDefaultSize { ...toggleProps }>


Before After

vipul0425 · 2024-09-11T05:47:37Z

packages/block-editor/src/components/global-styles/shadow-panel-components.js

@@ -89,8 +88,7 @@ export function ShadowIndicator( { type, label, isActive, onSelect, shadow } ) {
 } ) }
 render={
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before After

It looks a bit stretched to me after setting it to 40px. I tried using the small size, but then it appeared a bit too shrunken. Let me know if any adjustments are needed.

@WordPress/gutenberg-design ?

Seems to me like we just want normal <button>s (with the original size) here, not sure why use Button if these are fully custom.

It seems that the Shadow Palette has used the Button component since it was first implemented: https://github.com/WordPress/gutenberg/pull/46502/files#diff-d3db145a5a2d621586d0b1740cc8b403e58afac79bfe418bf7752fa3af88dc2bR164-R171

I don't know why the Button component was used, but based on this comment, it seems that we were trying to imitate the style of the color palette (CircularOptionPicker.Option component).

In fact, the CircularOptionPicker.Option component is also implemented internally as a Button component and is fully customized (source)

For this PR it's probably best to keep those buttons visually looking the same as before, regardless of what component is used. A button element rather than the component may be a fine solution there.

Longer term, we should find a better design for these that's clearer.

It would be good to avoid the arbitrary 26px sizing and use of the more standardised options (24, 32, or 40).

Hi @tyxla I have converted it to normal HTML buttons.

To maintain the previous appearance and functionality, I think the following problems need to be solved:

Apply none to the button's background color; otherwise, the browser's default gray background color (in Chrome) will be applied.

The checkmark position is incorrect when the shadow is selected and focused. We need to apply display:flex or display:inline-flex and align-items:center to the button.

We should use the Tooltip component to display text instead of the title attribute.

Two attributes, label and aria-label, are applied, but only aria-label should be needed.

The __next40pxDefaultSize prop will no longer be necessary.

We might need apperance: none to reset the browser styles just to be sure.

trunk This PR

I also noticed that wrapping the button with a Tooltip breaks the Composite's keyboard navigation, so we need to wrap the entire Composite.Item with the Tooltip.

☝️ @ciampo Did you know about this quirk? Not immediately sure why that is. The Tooltip embedded in a Button component doesn't break Composite.

No, I wasn't aware. Maybe we should open a separate issue to at least track this quirk

Hi @t-hamano I have updated <button> as per your suggestion. Let me know if anything else needs to be updated. 🙇

vipul0425 · 2024-09-11T05:49:45Z

packages/block-editor/src/components/global-styles/shadow-panel-components.js

@@ -42,8 +42,7 @@ export function ShadowPopoverContainer( { shadow, onShadowChange, settings } ) {
 />
 <div className="block-editor-global-styles__clear-shadow">
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before After

DaniGuardiola

LGTM except for the shadow buttons, see https://github.com/WordPress/gutenberg/pull/65225/files#r1755854718

Let's wait for design feedback.

…e-65018-3

mirka

I had one small nit, but everything else looks good now 👍

packages/block-library/src/group/editor.scss

Co-authored-by: Lena Morita <lena@jaguchi.com>

vipul0425 added 4 commits September 11, 2024 10:48

Fix: Global Styles component to use 40px default size.

70ec0f7

Fix: Block variation picket to use 40px default size.

631a42c

Fix: Block variation transform to use 40px default size.

36d6a70

Fix: color gradient dropdown and block appender button to use 40px de…

670cca8

…fault size.

vipul0425 requested a review from ellatrix as a code owner September 11, 2024 05:29

vipul0425 commented Sep 11, 2024

View reviewed changes

fix: shadowpanel clear button

913f90c

vipul0425 commented Sep 11, 2024

View reviewed changes

vipul0425 mentioned this pull request Sep 11, 2024

Button: Replace remaining 40px default size violations #65018

Open

21 tasks

tyxla requested a review from a team September 11, 2024 13:41

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

tyxla assigned vipul0425 Sep 11, 2024

DaniGuardiola reviewed Sep 11, 2024

View reviewed changes

vipul0425 added 3 commits September 16, 2024 13:20

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

e7b1615

…e-65018-3

fix: Button Block appender issues.

bb9545f

fix: Coverts shadow panel Buttons to normal html buttons.

c452baa

vipul0425 requested a review from ajitbohra as a code owner September 16, 2024 08:33

vipul0425 requested review from jameskoster, DaniGuardiola and tyxla September 16, 2024 08:37

mirka reviewed Sep 18, 2024

View reviewed changes

packages/block-library/src/group/editor.scss Outdated Show resolved Hide resolved

packages/block-library/src/group/editor.scss Outdated Show resolved Hide resolved

Update packages/block-library/src/group/editor.scss

c8c0e5d

Co-authored-by: Lena Morita <lena@jaguchi.com>

vipul0425 and others added 2 commits September 18, 2024 21:49

Update packages/block-library/src/group/editor.scss

00da47a

Co-authored-by: Lena Morita <lena@jaguchi.com>

feat: Add tootlip tu shadow button.

9d3d8ac

vipul0425 requested a review from t-hamano September 20, 2024 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Button Replace remaining 40px default size violations [Block Editor 3] #65225

Fix: Button Replace remaining 40px default size violations [Block Editor 3] #65225

vipul0425 commented Sep 11, 2024

github-actions bot commented Sep 11, 2024 •

edited

Loading

vipul0425 Sep 11, 2024

vipul0425 Sep 11, 2024

jameskoster Sep 12, 2024

DaniGuardiola Sep 12, 2024

jameskoster Sep 13, 2024

vipul0425 Sep 16, 2024

vipul0425 Sep 11, 2024

jameskoster Sep 12, 2024

vipul0425 Sep 16, 2024

vipul0425 Sep 11, 2024

vipul0425 Sep 11, 2024

vipul0425 Sep 11, 2024

vipul0425 Sep 11, 2024

vipul0425 Sep 11, 2024

DaniGuardiola Sep 11, 2024 •

edited

Loading

t-hamano Sep 12, 2024

jasmussen Sep 12, 2024

jameskoster Sep 12, 2024

vipul0425 Sep 16, 2024

t-hamano Sep 19, 2024

mirka Sep 19, 2024

ciampo Sep 19, 2024

vipul0425 Sep 20, 2024

vipul0425 Sep 11, 2024

DaniGuardiola left a comment

mirka left a comment

Fix: Button Replace remaining 40px default size violations [Block Editor 3] #65225

Are you sure you want to change the base?

Fix: Button Replace remaining 40px default size violations [Block Editor 3] #65225

Conversation

vipul0425 commented Sep 11, 2024

What?

Why?

How?

Testing Instructions

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniGuardiola Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniGuardiola left a comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 11, 2024 •

edited

Loading

DaniGuardiola Sep 11, 2024 •

edited

Loading