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

hbhalodia · 2024-09-05T05:31:57Z

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/navigation and core/navigation-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.

…size

…x size

github-actions · 2024-09-05T05:32:10Z

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: 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-05T05:34:35Z

packages/block-library/src/navigation-link/link-ui.js

@@ -310,8 +310,7 @@ const LinkUITools = ( { setAddingBlock, focusAddBlockButton } ) => {
 	return (
 		<VStack className="link-ui-tools">
 			<Button
-				// TODO: Switch to `true` (40px size) if possible
-				__next40pxDefaultSize={ false }
+				__next40pxDefaultSize


This can be seen, while you add navigation-link block and while you add the link.

Before After

hbhalodia · 2024-09-05T05:37:42Z

packages/block-library/src/navigation/edit/deleted-navigation-warning.js

@@ -16,8 +16,7 @@ function DeletedNavigationWarning( { onCreateNew } ) {
 				{
 					button: (
 						<Button
-							// TODO: Switch to `true` (40px size) if possible
-							__next40pxDefaultSize={ false }
+							__next40pxDefaultSize


This can be seen when you use navigation ref which is deleted or not present.

Also, this is kind of link which uses button component, but it has some overridden styles, so there would not be any effect on button. Still we have updated component to use default size, in future if specific styles get's removed, we have default 40px being applied.

Before After

hbhalodia · 2024-09-05T05:40:23Z

packages/block-library/src/navigation/edit/index.js

@@ -591,8 +591,7 @@ function Navigation( {
 						{ isResponsive && (
 							<>
 								<Button
-									// TODO: Switch to `true` (40px size) if possible
-									__next40pxDefaultSize={ false }
+									__next40pxDefaultSize


This can be seen on the inspector controls, when we add navigation block and we try to set overlay menu icon, refer to below screenshots.

Before After

We're going to need to preserve this height override. I think we'll need to add an !important here.

gutenberg/packages/block-library/src/navigation/editor.scss

Line 566 in 9523fb4

height: $grid-unit-40 * 2;

Aside: @WordPress/gutenberg-components This is already the second time I'm seeing a height override like this that requires an additional !important (first time was #65068 (comment)). I'm tempted to decrease the specificity of the rule (to &:where( .is-next-40px-default-size )), but I think we shouldn't because:

It could cause style regressions in existing usages.

The 40px opt-in prop (and thus the CSS class) is temporary.

If custom Button height like this is a legitimate design requirement, maybe it should be supported officially?

gutenberg/packages/components/src/button/style.scss

Lines 28 to 30 in 9523fb4

&.is-next-40px-default-size {

height: $button-size-next-default-40px;

}

We're going to need to preserve this height override. I think we'll need to add an !important here.

Thanks for this, would add !important to override the 40px style.

It's a tricky balance.

I lean on the side of not decreasing the specificity, especially since it's going to be removed soon anyway.

If custom Button height like this is a legitimate design requirement, maybe it should be supported officially?

I'm not sure that adding "one more variant" is a scalable solution.

Seeing how many heavily tweaked instances of Button we're encountering, I would consider (as already discussed in the past) to introduce an "unstyled" variant of Button, which would still benefit from the component's functionality (ie. tooltips, link vs button, focus styles, accessible when disabled, etc) but without any extra cosmetic changes applied (ie. no padding, margin, heighgits, etc).

Alternatively, we could expose a useButton hook which does the same thing, and use if with vanilla <button /> elements.

hbhalodia · 2024-09-05T05:43:15Z

packages/block-library/src/navigation/edit/navigation-menu-delete-control.js

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


This can be seen, while adding the navigation menu, going to inspector controls advanced tab, and check for Delete Menu button.

Before After

hbhalodia · 2024-09-05T05:46:27Z

packages/block-library/src/navigation/edit/responsive-wrapper.js

@@ -79,8 +79,7 @@ export default function ResponsiveWrapper( {
 		<>
 			{ ! isOpen && (
 				<Button
-					// TODO: Switch to `true` (40px size) if possible
-					__next40pxDefaultSize={ false }
+					__next40pxDefaultSize


This can be seen, while you add navigation and add the overlay menu icon to mobile or always show.

Note: For this we have some overriding styles on the icon itself, so it would not have before and after change, but we can see that this change have added our 40px change, so in future if overriding styles is removed, we have default 40px of size.

Before After

hbhalodia · 2024-09-05T05:50:01Z

packages/block-library/src/navigation/edit/responsive-wrapper.js

@@ -102,8 +101,7 @@ export default function ResponsiveWrapper( {
 				>
 					<div { ...dialogProps }>
 						<Button
-							// TODO: Switch to `true` (40px size) if possible
-							__next40pxDefaultSize={ false }
+							__next40pxDefaultSize


This can be seen, while you add navigation and add the overlay menu icon to mobile or always show, and open the menu by click on overlay icon, you can see the close icon.

Note: For this we have some overriding styles on the icon itself, so it would not have before and after change, but we can see that this change have added our 40px change, so in future if overriding styles is removed, we have default 40px of size.

Before After

hbhalodia · 2024-09-05T05:53:57Z

packages/block-library/src/navigation/edit/placeholder/index.js

@@ -76,8 +76,7 @@ export default function NavigationPlaceholder( {

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


I haven't being able to identify this change visually on the site, because the main component, NavigationPlaceholder is exported, but is not used anywhere in entire repository, Ref - https://github.com/search?q=repo%3AWordPress%2Fgutenberg%20export%20default%20function%20NavigationPlaceholder(&type=code,

It is being used here -

gutenberg/packages/block-library/src/navigation/edit/placeholder/index.js

Line 17 in 13af7fd

export default function NavigationPlaceholder( {

@mirka, Can you please verify this.

Thank You,

Yes, it doesn't seem to be used anymore 👍

Thanks for confirmation 👍.

mirka

There's one issue we need to fix in the Navigation block inspector, but everything else looks good. Thank you!

mirka · 2024-09-05T22:52:37Z

packages/block-library/src/navigation/edit/index.js

@@ -591,8 +591,7 @@ function Navigation( {
 						{ isResponsive && (
 							<>
 								<Button
-									// TODO: Switch to `true` (40px size) if possible
-									__next40pxDefaultSize={ false }
+									__next40pxDefaultSize


We're going to need to preserve this height override. I think we'll need to add an !important here.

gutenberg/packages/block-library/src/navigation/editor.scss

Line 566 in 9523fb4

height: $grid-unit-40 * 2;

Aside: @WordPress/gutenberg-components This is already the second time I'm seeing a height override like this that requires an additional !important (first time was #65068 (comment)). I'm tempted to decrease the specificity of the rule (to &:where( .is-next-40px-default-size )), but I think we shouldn't because:

It could cause style regressions in existing usages.

The 40px opt-in prop (and thus the CSS class) is temporary.

If custom Button height like this is a legitimate design requirement, maybe it should be supported officially?

gutenberg/packages/components/src/button/style.scss

Lines 28 to 30 in 9523fb4

&.is-next-40px-default-size {

height: $button-size-next-default-40px;

}

mirka · 2024-09-05T23:34:49Z

packages/block-library/src/navigation/edit/placeholder/index.js

@@ -76,8 +76,7 @@ export default function NavigationPlaceholder( {

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


Yes, it doesn't seem to be used anymore 👍

Ref - WordPress#65075 (comment)

hbhalodia · 2024-09-06T05:42:37Z

Hi @mirka, I have updated the PR with the requested changes. I have requested for re-review.

Thank You,

mirka

Thanks, I appreciate you being proactive about adding a code comment!

…rary] (#65075) * Fix navigation-link block to use button to default 40px size * Fix navigation deleted create new menu button to use 40px default size * Fix navigation overlay menu icon button to use 40px default size * Fix the navigation menu delete button to use 40px default size button * Fix navigation placeholder start empty to use button as default 40px size * Fix responsive navigation overlay menu open button to use default 40px size * Fix navigation responsive close button to use 40px default size * Add the !important to override the overlay menu preview style Ref - WordPress/gutenberg#65075 (comment) * Refactor the comment for the !important style Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Source: WordPress/gutenberg@219be48

hbhalodia added 7 commits September 4, 2024 20:40

Fix navigation-link block to use button to default 40px size

d1e1371

Fix navigation deleted create new menu button to use 40px default size

d41604c

Fix navigation overlay menu icon button to use 40px default size

6d2d3df

Fix the navigation menu delete button to use 40px default size button

3edc766

Fix navigation placeholder start empty to use button as default 40px …

338d4b3

…size

Fix responsive navigation overlay menu open button to use default 40p…

f0febe5

…x size

Fix navigation responsive close button to use 40px default size

977b741

hbhalodia requested a review from ajitbohra as a code owner September 5, 2024 05:31

hbhalodia commented Sep 5, 2024

View reviewed changes

hbhalodia mentioned this pull request Sep 5, 2024

Button: Replace remaining 40px default size violations #65018

Closed

21 tasks

tyxla requested a review from a team September 5, 2024 12:31

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

mirka reviewed Sep 5, 2024

View reviewed changes

mirka assigned hbhalodia Sep 5, 2024

hbhalodia added 2 commits September 6, 2024 11:08

Add the !important to override the overlay menu preview style

ecf4b99

Ref - WordPress#65075 (comment)

Refactor the comment for the !important style

7c3d35c

hbhalodia requested a review from mirka September 6, 2024 05:42

mirka approved these changes Sep 6, 2024

View reviewed changes

mirka merged commit 219be48 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 violation [Block library] #65075

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

hbhalodia commented Sep 5, 2024

github-actions bot commented Sep 5, 2024 •

edited

Loading

hbhalodia Sep 5, 2024

hbhalodia Sep 5, 2024

hbhalodia Sep 5, 2024

mirka Sep 5, 2024

hbhalodia Sep 6, 2024

ciampo Sep 6, 2024

hbhalodia Sep 5, 2024

hbhalodia Sep 5, 2024

hbhalodia Sep 5, 2024 •

edited

Loading

hbhalodia Sep 5, 2024 •

edited

Loading

mirka Sep 5, 2024

hbhalodia Sep 6, 2024

mirka left a comment

mirka Sep 5, 2024

mirka Sep 5, 2024

hbhalodia commented Sep 6, 2024

mirka left a comment

	&.is-next-40px-default-size {
	height: $button-size-next-default-40px;
	}

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

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

Conversation

hbhalodia commented Sep 5, 2024

What?

Why?

How?

Testing Instructions

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

hbhalodia Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

hbhalodia Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbhalodia commented Sep 6, 2024

mirka left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 5, 2024 •

edited

Loading

hbhalodia Sep 5, 2024 •

edited

Loading

hbhalodia Sep 5, 2024 •

edited

Loading