Add __next40pxDefaultSize for files in editor 3 #65139

AKSHAT2802 · 2024-09-08T20:15:39Z

Part of - #65018

What?

Issue - #65018, To use default to 40px for the button.

Why?

To make the consistent button across Gutenberg, and we would have a lint rule added once fixed, all the button usage.

How?

Add __next40pxDefaultSize in button on the component.

Testing Instructions

Screenshots added as comments in file changes

github-actions · 2024-09-08T20:15:54Z

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: AKSHAT2802 <akshat2802@git.wordpress.org>
Co-authored-by: tyxla <tyxla@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.

github-actions · 2024-09-08T20:16:07Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @AKSHAT2802! 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.

AKSHAT2802 · 2024-09-08T20:36:47Z

packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js

@@ -428,8 +428,7 @@ export function HierarchicalTermSelector( { slug } ) {
 			{ ! loading && hasCreateAction && (
 				<FlexItem>
 					<Button
-						// TODO: Switch to `true` (40px size) if possible
-						__next40pxDefaultSize={ false }
+						__next40pxDefaultSize


Before After

Note:- button variant set to link.

Looks good 👍

@mirka, it felt off that we're forcing the size prop here. After all variant="link" buttons override the height to auto anyway? We're almost done with this migration, but if we were just beginning with it, maybe it would have made sense to exclude the variant="link" buttons from enforcing the size prop.

Yeah, that makes sense. We'll make sure to exclude these when we start throwing deprecation warnings in console.

AKSHAT2802 · 2024-09-08T20:38:13Z

packages/editor/src/components/post-taxonomies/most-used-terms.js

@@ -62,8 +62,7 @@ export default function MostUsedTerms( { onSelect, taxonomy } ) {
 				{ terms.map( ( term ) => (
 					<li key={ term.id }>
 						<Button
-							// TODO: Switch to `true` (40px size) if possible
-							__next40pxDefaultSize={ false }
+							__next40pxDefaultSize


Before After

Note: Button variant set to link.

Ah, we have an awkward override here:

gutenberg/packages/editor/src/components/post-taxonomies/style.scss

Lines 38 to 40 in ecbde0e

.components-button {

font-size: 12px;

}

My preference: I'd just remove that override, and go with the 13px. The default font size is 13px, so it wouldn't make much difference.

If we really wanted to make it smaller, we could use the size="small" prop, although I don't see a good reason to do it here.

AKSHAT2802 · 2024-09-08T20:41:24Z

packages/editor/src/components/post-url/index.js

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


Before After

This was already defacto addressed as part of #64824 - InputControl would pass the __next40pxDefaultSize size prop along to the prefix and suffix component, and the underlying Button was obtaining it.

It's likely you were using an old trunk when making the Before screenshot. In my testing, I see a 40px height button already, with and without this change. However, for consistency, it only makes sense we pass that prop here.

So, nothing to do here 👍

Actually these screenshots are of a different UI. This file renders this UI here:

☝️ Since this is in an InputControl prefix slot, we want this copy button to be size="small", and wrap it in a <InputControlSuffixWrapper variant="control">.

Please see this With Icon Or Control story for an example.

tyxla

Thanks for working on this one @AKSHAT2802 🙌

Things mostly look good, but there are some opportunities for cleanup.

My biggest concern is the lack of consistency with the rest of the PostSummary buttons if we merge this one separately. Should we merge together with the rest? cc @mirka @ciampo

tyxla · 2024-09-11T12:29:48Z

packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js

@@ -428,8 +428,7 @@ export function HierarchicalTermSelector( { slug } ) {
 			{ ! loading && hasCreateAction && (
 				<FlexItem>
 					<Button
-						// TODO: Switch to `true` (40px size) if possible
-						__next40pxDefaultSize={ false }
+						__next40pxDefaultSize


Looks good 👍

@mirka, it felt off that we're forcing the size prop here. After all variant="link" buttons override the height to auto anyway? We're almost done with this migration, but if we were just beginning with it, maybe it would have made sense to exclude the variant="link" buttons from enforcing the size prop.

tyxla · 2024-09-11T12:33:37Z

packages/editor/src/components/post-taxonomies/most-used-terms.js

@@ -62,8 +62,7 @@ export default function MostUsedTerms( { onSelect, taxonomy } ) {
 				{ terms.map( ( term ) => (
 					<li key={ term.id }>
 						<Button
-							// TODO: Switch to `true` (40px size) if possible
-							__next40pxDefaultSize={ false }
+							__next40pxDefaultSize


Ah, we have an awkward override here:

gutenberg/packages/editor/src/components/post-taxonomies/style.scss

Lines 38 to 40 in ecbde0e

.components-button {

font-size: 12px;

}

My preference: I'd just remove that override, and go with the 13px. The default font size is 13px, so it wouldn't make much difference.

If we really wanted to make it smaller, we could use the size="small" prop, although I don't see a good reason to do it here.

packages/editor/src/components/post-template/classic-theme.js

tyxla · 2024-09-11T13:07:01Z

packages/editor/src/components/post-url/index.js

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


This was already defacto addressed as part of #64824 - InputControl would pass the __next40pxDefaultSize size prop along to the prefix and suffix component, and the underlying Button was obtaining it.

It's likely you were using an old trunk when making the Before screenshot. In my testing, I see a 40px height button already, with and without this change. However, for consistency, it only makes sense we pass that prop here.

So, nothing to do here 👍

packages/editor/src/components/save-publish-panels/index.js

packages/editor/src/components/post-taxonomies/style.scss

…t 13px size in style as it is default

AKSHAT2802 · 2024-09-13T17:48:32Z

Hii @mirka @tyxla
I have removed redundant style for font-size: 13px; and reverted change for classic-theme.js file as it can be treated separately according to this.

mirka · 2024-09-16T10:43:01Z

@AKSHAT2802 Could we also address this one please? #65139 (comment)

mirka

Perfect, thanks!

Add __next40pxDefaultSize for files in editor 3

a3803a3

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

AKSHAT2802 commented Sep 8, 2024

View reviewed changes

shail-mehta added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 9, 2024

tyxla requested a review from a team September 10, 2024 13:13

tyxla assigned tyxla and AKSHAT2802 and unassigned tyxla Sep 10, 2024

tyxla reviewed Sep 11, 2024

View reviewed changes

tyxla mentioned this pull request Sep 11, 2024

Button: Replace remaining 40px default size violations #65018

Closed

21 tasks

AKSHAT2802 added 2 commits September 12, 2024 01:48

Address feedback

8496476

change font size to 13 px

f8c3df5

tyxla reviewed Sep 11, 2024

View reviewed changes

packages/editor/src/components/post-taxonomies/style.scss Outdated Show resolved Hide resolved

Revert classic-theme.js changes as it will be a seperate PR and rever…

ec41ae5

…t 13px size in style as it is default

AKSHAT2802 requested review from tyxla and ciampo September 12, 2024 10:13

Wrap copy button in InputControlSuffixWrapper

ea59cae

mirka approved these changes Sep 17, 2024

View reviewed changes

mirka enabled auto-merge (squash) September 17, 2024 04:53

mirka merged commit f82bc32 into WordPress:trunk Sep 17, 2024
59 of 60 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add __next40pxDefaultSize for files in editor 3 #65139

Add __next40pxDefaultSize for files in editor 3 #65139

AKSHAT2802 commented Sep 8, 2024

github-actions bot commented Sep 8, 2024 •

edited

Loading

github-actions bot commented Sep 8, 2024

AKSHAT2802 Sep 8, 2024

tyxla Sep 11, 2024

mirka Sep 13, 2024

AKSHAT2802 Sep 8, 2024

tyxla Sep 11, 2024

AKSHAT2802 Sep 8, 2024

tyxla Sep 11, 2024

mirka Sep 13, 2024

tyxla left a comment

tyxla Sep 11, 2024

tyxla Sep 11, 2024

tyxla Sep 11, 2024

AKSHAT2802 commented Sep 13, 2024

mirka commented Sep 16, 2024

mirka left a comment

Add __next40pxDefaultSize for files in editor 3 #65139

Add __next40pxDefaultSize for files in editor 3 #65139

Conversation

AKSHAT2802 commented Sep 8, 2024

What?

Why?

How?

Testing Instructions

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

github-actions bot commented Sep 8, 2024

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

tyxla left a 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

AKSHAT2802 commented Sep 13, 2024

mirka commented Sep 16, 2024

mirka left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2024 •

edited

Loading