Fix: Button Replace remaining 40px default size violations [Block Editor 3] by vipul0425 · Pull Request #65225 · WordPress/gutenberg

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

 					<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

 				<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

 					<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

 									<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

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

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

Thanks for the update. I think it perfectly maintains its functionality and style 👍

Documented the Composite + Tooltip issue at #65615.

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

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

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

mirka

Looks great, thank you for the follow-ups!

noisysocks · 2024-10-06T21:23:47Z

When I try to cherry-pick df5eaed onto wp/6.7 I get an empty commit. I suppose it was already backported? Just double check that please and if everything's okay remove the label.

…tor 3] (#65225) * Fix: Global Styles component to use 40px default size. * Fix: Block variation picket to use 40px default size. * Fix: Block variation transform to use 40px default size. * Fix: color gradient dropdown and block appender button to use 40px default size. * fix: shadowpanel clear button * fix: Button Block appender issues. * fix: Coverts shadow panel Buttons to normal html buttons. * Update packages/block-library/src/group/editor.scss Co-authored-by: Lena Morita <lena@jaguchi.com> * Update packages/block-library/src/group/editor.scss Co-authored-by: Lena Morita <lena@jaguchi.com> * feat: Add tootlip tu shadow button. --------- 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>

mirka · 2024-10-07T13:20:44Z

@noisysocks I'm confused, it looks like you already backported it for us in 53b4edf ?

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

Closed

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 DaniGuardiola, jameskoster 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

mirka approved these changes Sep 24, 2024

View reviewed changes

mirka merged commit df5eaed into WordPress:trunk Sep 24, 2024

github-actions bot added this to the Gutenberg 19.4 milestone Sep 24, 2024

This was referenced Oct 1, 2024

Social Icons: incorrect block appender size #65766

Closed

Social Links: Fix block appender size #65769

Merged

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

noisysocks mentioned this pull request Oct 6, 2024

Update packages for 6.7 Beta 2 WordPress/wordpress-develop#7515

Closed

kevin940726 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 11, 2024

draganescu mentioned this pull request Oct 15, 2024

Manual cherry pick for #65627 #66119

Merged

aaronrobertshaw mentioned this pull request Oct 17, 2024

Navigation block: fix block appender size #66153

Merged

sirreal mentioned this pull request Oct 25, 2024

test/pr automation workflow #66457

Closed

Conversation

vipul0425 commented Sep 11, 2024

What?

Why?

How?

Testing Instructions

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaniGuardiola Sep 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaniGuardiola left a comment

Choose a reason for hiding this comment

Uh oh!

mirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mirka left a comment

Choose a reason for hiding this comment

Uh oh!

noisysocks commented Oct 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirka commented Oct 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

github-actions bot commented Sep 11, 2024 •

edited

Loading

DaniGuardiola Sep 11, 2024 •

edited

Loading

noisysocks commented Oct 6, 2024 •

edited

Loading