Fix: Button: Replace remaining 40px default size violation [Block library 3] #65110

hbhalodia · 2024-09-06T06:26:08Z

Part of - #65018

What?

Issue - Button: Replace remaining 40px default size violations #65018, To use default to 40px for the button.
This would fix in the blocks, core/page-list, core/site-logo, core/post-featured-image, core/post-comments-form and core/social-link

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

Add the blocks on the page/post and check for the buttons defined.
Screenshot is added for individual changed files.

…ault size

…t sizee

github-actions · 2024-09-06T06:26:22Z

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: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

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

hbhalodia · 2024-09-06T06:28:07Z

packages/block-library/src/page-list/convert-to-links-modal.js

@@ -32,16 +32,14 @@ export function ConvertToLinksModal( { onClick, onClose, disabled } ) {
 			</p>
 			<div className="wp-block-page-list-modal-buttons">
 				<Button
-					// TODO: Switch to `true` (40px size) if possible
-					__next40pxDefaultSize={ false }
+					__next40pxDefaultSize


This can be seen, while you add page-list block and try to convert page-list to navigationLinks

Before After

hbhalodia · 2024-09-06T06:28:19Z

packages/block-library/src/page-list/convert-to-links-modal.js

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


This can be seen, while you add page-list block and try to convert page-list to navigationLinks

Before After

hbhalodia · 2024-09-06T06:30:02Z

packages/block-library/src/page-list/edit.js

@@ -342,8 +342,7 @@ export default function PageListEdit( {
 					<PanelBody title={ __( 'Edit this menu' ) }>
 						<p>{ convertDescription }</p>
 						<Button
-							// TODO: Switch to `true` (40px size) if possible
-							__next40pxDefaultSize={ false }
+							__next40pxDefaultSize


This can be seen, while you add page-list block and check the inspector panels, when convertToLinks option is true.

Before After

hbhalodia · 2024-09-06T06:31:27Z

packages/block-library/src/post-comments-form/form.js

@@ -84,8 +84,7 @@ const CommentsForm = ( { postId, postType } ) => {
 		if ( 'closed' === commentStatus ) {
 			const actions = [
 				<Button
-					// TODO: Switch to `true` (40px size) if possible
-					__next40pxDefaultSize={ false }
+					__next40pxDefaultSize


This can be seen, while you add core/post-comments-form block, and you have comments disabled.

Before After

hbhalodia · 2024-09-06T06:33:32Z

packages/block-library/src/post-featured-image/edit.js

@@ -308,8 +308,7 @@ export default function PostFeaturedImageEdit( {
 				mediaLibraryButton={ ( { open } ) => {
 					return (
 						<Button
-							// TODO: Switch to `true` (40px size) if possible
-							__next40pxDefaultSize={ false }
+							__next40pxDefaultSize


This can be seen, while you add core/post-featured-image block.

Note: This has some styles that have higher specificity and specific to this upload button on this block, hence before and after is same, but we can see our change is applied in inspector controls.

Before After

As @hbhalodia said, noting that the __next40pxDefaultSize prop here doesn't have an impact because of the style overrides applied to the placeholder and the button components in the post featured image block.

These style overrides should be removed, if possible — but it's not in the scope of this PR. We can keep changes as they are.

hbhalodia · 2024-09-06T06:35:43Z

packages/block-library/src/site-logo/edit.js

@@ -627,8 +627,7 @@ export default function LogoEdit( {
 								render={ ( { open } ) => (
 									<div className="block-library-site-logo__inspector-upload-container">
 										<Button
-											// TODO: Switch to `true` (40px size) if possible
-											__next40pxDefaultSize={ false }
+											__next40pxDefaultSize


This can be seen, while you add core/site-logo block, and check the inspector controls to upload the logo.

Note: This has some styles that have higher specificity and specific to this button on this block, hence before and after is same, but we can see our change is applied in inspector controls.

Before After

We have style overrides in this case as well.

For the context of this PR, I think the current code changes are enough.

Still, I'd love to remove all of those overrides if possible, and just use a standard Button variant — although the situation is trickier than it looks. There are three possible UIs shown to the user:

an upload button (the one in the image above)

a "readonly" item group showing the logo preview;

an interactive dropdown allowing to replace/remove the current logo

Therefore, we'd need to make sure that we could offer a good, consistent UI in all three cases even without those overrides. Something we could do in a follow-up (cc @WordPress/gutenberg-design )/

Also, I noticed that the dropdown comes from the MediaReplaceFlow component. That dropdown is using a ToolbarButton as its trigger, showing that it was only meant to be rendered in a toolbar — while here it's being used in the editor sidebar (cc @WordPress/gutenberg-components @ntsekouras @youknowriad ).

hbhalodia · 2024-09-06T06:37:24Z

packages/block-library/src/site-logo/edit.js

@@ -674,8 +673,7 @@ export default function LogoEdit( {
 					mediaLibraryButton={ ( { open } ) => {
 						return (
 							<Button
-								// TODO: Switch to `true` (40px size) if possible
-								__next40pxDefaultSize={ false }
+								__next40pxDefaultSize


This can be seen, while you add core/site-logo block, check for upload button.

Note: This has some styles that have higher specificity and specific to this upload button on this block, hence before and after is same, but we can see our change is applied in inspector controls.

Before After

Same as above. There are again a lot of style overrides, which ideally shouldn't be there at all.

The code change here is good for the scope of this PR

hbhalodia · 2024-09-06T06:39:03Z

packages/block-library/src/social-link/edit.js

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


This can be seen, while you add core/social-link block, and add the link. You can check for the apply button.

Before After

We should update the URLInput to a 40px height too — @mirka , do you know if that's feasible as of now? Or should we keep the TODO comment and revisit later?

Mmm, this is kind of dependent on #64709, so I guess we can revert to TODO for now. I'll do something about it if the Button migrations get completed before the URLInput issue is addressed.

Sure, I would update this to TODO state.

ciampo

Thank you for working on this!

Most of the changes LGTM to me for the scope of this PR. The only pending item is #65110 (comment)

hbhalodia · 2024-09-07T10:57:47Z

Thank you for working on this!

Most of the changes LGTM to me for the scope of this PR. The only pending item is #65110 (comment)

Hi @ciampo @mirka, the PR is updated with the feedbacks.

Thank You,

ciampo

Changes LGTM given the scope of this PR.

Thank you, @hbhalodia 🙏

hbhalodia added 6 commits September 6, 2024 11:23

Fix the modal button for page list to use default 40px button size

ff5e4d5

Fix convert page list to navigation links edit button to use 40px def…

cfd8c15

…ault size

Fix enable comments form to use 40px default button size

3c7bd33

Fix featured image block open media library button to use 40px defaul…

87a37b7

…t sizee

Fix site-logo block to use 40px default button size

f5ddf47

Fix social link apply link button to use 40px default size

0d3029d

hbhalodia requested a review from ajitbohra as a code owner September 6, 2024 06:26

hbhalodia commented Sep 6, 2024

View reviewed changes

hbhalodia mentioned this pull request Sep 6, 2024

Button: Replace remaining 40px default size violations #65018

Closed

21 tasks

akasunil assigned hbhalodia Sep 6, 2024

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

ciampo requested a review from a team September 6, 2024 11:28

ciampo reviewed Sep 6, 2024

View reviewed changes

Revert social link apply button size to todo state

0e297d9

hbhalodia requested review from mirka and ciampo September 7, 2024 10:57

ciampo approved these changes Sep 9, 2024

View reviewed changes

ciampo merged commit 168d3c5 into WordPress:trunk Sep 9, 2024
62 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Button: Replace remaining 40px default size violation [Block library 3] #65110

Fix: Button: Replace remaining 40px default size violation [Block library 3] #65110

hbhalodia commented Sep 6, 2024

github-actions bot commented Sep 6, 2024 •

edited

Loading

hbhalodia Sep 6, 2024

hbhalodia Sep 6, 2024

hbhalodia Sep 6, 2024

hbhalodia Sep 6, 2024

hbhalodia Sep 6, 2024

ciampo Sep 6, 2024

hbhalodia Sep 6, 2024

ciampo Sep 6, 2024

hbhalodia Sep 6, 2024

ciampo Sep 6, 2024

hbhalodia Sep 6, 2024

ciampo Sep 6, 2024

mirka Sep 6, 2024

hbhalodia Sep 7, 2024

ciampo left a comment

hbhalodia commented Sep 7, 2024

ciampo left a comment

Fix: Button: Replace remaining 40px default size violation [Block library 3] #65110

Fix: Button: Replace remaining 40px default size violation [Block library 3] #65110

Conversation

hbhalodia commented Sep 6, 2024

What?

Why?

How?

Testing Instructions

github-actions bot commented Sep 6, 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

ciampo left a comment

Choose a reason for hiding this comment

hbhalodia commented Sep 7, 2024

ciampo left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2024 •

edited

Loading