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

PARTHVATALIYA · 2024-09-16T06:58:29Z

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 · 2024-09-16T06:58:44Z

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>

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

github-actions · 2024-09-16T06:58:49Z

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

PARTHVATALIYA · 2024-09-16T07:08:09Z

packages/block-editor/src/components/inspector-controls-tabs/index.js

@@ -49,8 +49,7 @@ export default function InspectorControlsTabs( {
 tabId={ tab.name }
 render={
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

__next40pxDefaultSize has no effect here because the parent div has default height: 48px

It would be tricky to get rid of all the overrides here. I wonder if it even makes sense to use a Button component for such a heavily customized use case.

Agreed. Could we try to switch to a regular button ? Since Tabs.Tab already renders a button by default, we could avoid using the render prop altogether.

PARTHVATALIYA · 2024-09-16T07:12:46Z

packages/block-editor/src/components/inspector-popover-header/index.js

@@ -31,8 +31,7 @@ export default function InspectorPopoverHeader( {
 <Spacer />
 { actions.map( ( { label, icon, onClick } ) => (
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

__next40pxDefaultSize has no effect here because .block-editor-inspector-popover-header__action class has height: 24px CSS

Could we consider removing the style override, and setting size="small" instead?

Hello @ciampo After adding size="small", .block-editor-inspector-popover-header__action class still override the height=24px

Yup! Part of my suggestion above was also to remove the style overrides

PARTHVATALIYA · 2024-09-16T07:14:59Z

packages/block-editor/src/components/inspector-popover-header/index.js

@@ -45,8 +44,7 @@ export default function InspectorPopoverHeader( {
 ) ) }
 { onClose && (
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

__next40pxDefaultSize has no effect here because .block-editor-inspector-popover-header__action class has height: 24px CSS

Could we consider removing the style override, and setting size="small" instead?

Hello @ciampo After adding size="small", .block-editor-inspector-popover-header__action class still override the height=24px

See #65361 (comment)

PARTHVATALIYA · 2024-09-16T07:19:46Z

packages/block-editor/src/components/link-control/index.js

@@ -408,8 +408,7 @@ function LinkControl( {
 { ! showActions && (
 <div className="block-editor-link-control__search-enter">
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

Doesn't look right. We'll need to fix the positioning to match up on the original.

Let's also test creating a link in a standard paragraph block 🙏

Test creating link in a standard paragraph block.

Before:

After:

Turns out we'll be addressing this file in #65158, so you can ignore it here 👍

PARTHVATALIYA · 2024-09-16T07:20:24Z

packages/block-editor/src/components/link-control/index.js

@@ -470,16 +469,14 @@ function LinkControl( {
 className="block-editor-link-control__search-actions"
 >
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

PARTHVATALIYA · 2024-09-16T07:21:20Z

packages/block-editor/src/components/link-control/index.js

 variant="tertiary"
 onClick={ handleCancel }
 >
 { __( 'Cancel' ) }
 </Button>
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

PARTHVATALIYA · 2024-09-16T07:23:08Z

packages/block-editor/src/components/link-control/settings-drawer.js

@@ -25,8 +25,7 @@ function LinkSettingsDrawer( { children, settingsOpen, setSettingsOpen } ) {
 return (
 <>
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

I think this one looks good 👍

PARTHVATALIYA · 2024-09-16T07:26:36Z

packages/block-editor/src/components/list-view/block-select-button.js

@@ -83,8 +83,7 @@ function ListViewBlockSelectButton(

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


Before:

After:

__next40pxDefaultSize has no effect here because .block-editor-list-view-block-contents class has height: 32px CSS

Could we consider removing the style override, and setting size="compact" instead?

PARTHVATALIYA · 2024-09-16T07:33:55Z

packages/block-editor/src/components/media-placeholder/index.js

@@ -51,8 +51,7 @@ const InsertFromURLPopover = ( {
 value={ src }
 />
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

@mirka will this be also affected by #65158 ?

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

#65158 has landed, so after merging latest trunk into this branch, we can move the button to the suffix slot.

PARTHVATALIYA · 2024-09-16T07:35:33Z

packages/block-editor/src/components/media-placeholder/index.js

@@ -87,8 +86,7 @@ const URLSelectionUI = ( { src, onChangeSrc, onSelectURL } ) => {
 return (
 <div className="block-editor-media-placeholder__url-input-container">
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

PARTHVATALIYA · 2024-09-16T07:36:53Z

packages/block-editor/src/components/media-placeholder/index.js

@@ -419,8 +416,7 @@ export function MediaPlaceholder( {
 onToggleFeaturedImage && (
 <div className="block-editor-media-placeholder__url-input-container">
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

PARTHVATALIYA · 2024-09-16T07:37:23Z

packages/block-editor/src/components/media-placeholder/index.js

@@ -436,8 +432,7 @@ export function MediaPlaceholder( {
 const defaultButton = ( { open } ) => {
 return (
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

PARTHVATALIYA · 2024-09-16T07:37:48Z

packages/block-editor/src/components/media-placeholder/index.js

@@ -508,8 +502,7 @@ export function MediaPlaceholder( {
 <FormFileUpload
 render={ ( { openFileDialog } ) => (
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

PARTHVATALIYA · 2024-09-16T09:05:44Z

packages/block-editor/src/components/media-placeholder/index.js

@@ -389,8 +387,7 @@ export function MediaPlaceholder( {
 return (
 onCancel && (
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

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

getdave

Thanks for the ping. Link Control looks like it might need some tweaks.

getdave · 2024-09-16T09:05:40Z

packages/block-editor/src/components/link-control/index.js

@@ -408,8 +408,7 @@ function LinkControl( {
 { ! showActions && (
 <div className="block-editor-link-control__search-enter">
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Doesn't look right. We'll need to fix the positioning to match up on the original.

getdave · 2024-09-16T09:05:57Z

packages/block-editor/src/components/link-control/settings-drawer.js

@@ -25,8 +25,7 @@ function LinkSettingsDrawer( { children, settingsOpen, setSettingsOpen } ) {
 return (
 <>
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


I think this one looks good 👍

getdave · 2024-09-16T09:06:35Z

packages/block-editor/src/components/link-control/index.js

@@ -408,8 +408,7 @@ function LinkControl( {
 { ! showActions && (
 <div className="block-editor-link-control__search-enter">
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Let's also test creating a link in a standard paragraph block 🙏

PARTHVATALIYA · 2024-09-16T09:14:54Z

packages/block-editor/src/components/media-placeholder/index.js

@@ -477,8 +472,7 @@ export function MediaPlaceholder( {
 const content = (
 <>
 <Button
- // TODO: Switch to `true` (40px size) if possible
- __next40pxDefaultSize={ false }
+ __next40pxDefaultSize


Before:

After:

tyxla

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.

Fix: inspector control tabs to use 40px default button size

246fe1b

PARTHVATALIYA requested a review from ellatrix as a code owner September 16, 2024 06:58

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

PARTHVATALIYA mentioned this pull request Sep 16, 2024

Button: Replace remaining 40px default size violations #65018

Open

21 tasks