Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Button Replace remaining 40px default size violations [Block Editor 1]. #65034

Merged
merged 10 commits into from
Sep 6, 2024

Conversation

vipul0425
Copy link
Contributor

Part of - #65018

What?

  • This PR adds the 40px default size to invalid block warning, multiple block usage warning, block compare, block breadcrumbs and block mover.

Why?

To make the consistent button across Gutenberg.

How?

Change from __next40pxDefaultSize={ false } to __next40pxDefaultSize on component.

  • Testing steps and screenshots are added below.

Copy link

github-actions bot commented Sep 4, 2024

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: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>

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

@@ -31,8 +31,7 @@ export default function BlockView( {

<div className="block-editor-block-compare__action">
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be seen when any block as a validation error and you click on resolve.

Before After
block compare 36 block compare 40px

@@ -24,17 +24,15 @@ export function MultipleUsageWarning( {
<Warning
actions={ [
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This occurs when a block has multiple supports set to false in block.json and you add that block multiple times in the editor.

Before After
multiple usage warning 36 multiple usage warning 40px

@@ -101,8 +101,7 @@ export default function BlockInvalidWarning( { clientId } ) {
<Warning
actions={ [
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The invalid warning can be seen when a block's content is invalid after saving the page and then returning to it.

Before After
block validation 36 block invalidation 40px

@vipul0425
Copy link
Contributor Author

vipul0425 commented Sep 4, 2024

Hi @mirka , for the block breadcrumbs and block mover, I have set __next40pxDefaultSize to true, but the custom styles defined in those blocks are overriding it to 20px. Should we update those custom styles as well?

@ciampo
Copy link
Contributor

ciampo commented Sep 4, 2024

Hi @mirka , for the block breadcrumbs and block mover, I have set __next40pxDefaultSize to true, but the custom styles defined in those blocks are overriding it to 20px. Should we update those custom styles as well?

Mhh, good question!

Overriding the height shouldn't happen in the first place.

For the block breadcrumbs, we could consider setting size="small", which would change the height to 24px — although I'm aware that screen real estate is precious and we may not want to increase their height.

In both cases, an alternative could also be to use vanilla <button /> elements in combination with <Icon > and <Tooltip /> where needed, and do extra work to make them accessibleWhenDisabled.

We should also consider that all <Button />s in the block toolbar also have their height overridden to 48px anyway.

I'm not sure if neither of those approaches are better than the CSS height override, though — I'd love to hear @WordPress/gutenberg-components and @WordPress/gutenberg-design 's thoughts.

@ciampo ciampo added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Package] Block editor /packages/block-editor labels Sep 4, 2024
@jameskoster
Copy link
Contributor

On trunk the breadcrumb buttons seem to have a style override to make them 24px:

Screenshot 2024-09-04 at 16 18 07

Removing that and using the small size makes sense to me.

For the movers; there appears to be space to make them 24px with a couple of adjustments to maintain visual appearance. Again, could we use the small size and remove the override?

@ciampo
Copy link
Contributor

ciampo commented Sep 4, 2024

@vipul0425 , could try to set size="small" on both instances and remove the height style override?

@vipul0425
Copy link
Contributor Author

vipul0425 commented Sep 5, 2024

Hi @ciampo For the breadcrumbs it is working fine.

image

However, when I apply that to the block mover buttons and remove the height-specific styles from its stylesheet, the height set by the toolbar component has higher specificity and takes priority. Should we override this in the block-mover button styles using $button-size-small, or is there another approach you'd recommend?

@jasmussen
Copy link
Contributor

Thanks for working on this!

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping with the button sizing @vipul0425 🙌

Note that some snapshot tests will be failing as a consequence, and you might have to run npm run test:unit:update packages/block-editor/src/components/ to update them and then commit them as part of the PR.

@tyxla tyxla added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Enhancement A suggestion for improvement. labels Sep 5, 2024
@@ -23,7 +23,6 @@
}

.block-editor-block-breadcrumb__button.components-button {
height: $button-size-small;
Copy link
Contributor

@ciampo ciampo Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can potentially remove a lot more style overrides (and the .components-button classname, which should not be used at all). It also looks like the tertiary variant is not really needed.

Something like this (click to show diff)
diff --git a/packages/block-editor/src/components/block-breadcrumb/index.js b/packages/block-editor/src/components/block-breadcrumb/index.js
index 865d8d0600..be0ab9fc45 100644
--- a/packages/block-editor/src/components/block-breadcrumb/index.js
+++ b/packages/block-editor/src/components/block-breadcrumb/index.js
@@ -68,7 +68,6 @@ function BlockBreadcrumb( { rootLabelText } ) {
 					<Button
 						size="small"
 						className="block-editor-block-breadcrumb__button"
-						variant="tertiary"
 						onClick={ () => {
 							// Find the block editor wrapper for the selected block
 							const blockEditor = blockRef.current?.closest(
diff --git a/packages/block-editor/src/components/block-breadcrumb/style.scss b/packages/block-editor/src/components/block-breadcrumb/style.scss
index a1ae3bf2f1..af56971fe4 100644
--- a/packages/block-editor/src/components/block-breadcrumb/style.scss
+++ b/packages/block-editor/src/components/block-breadcrumb/style.scss
@@ -22,39 +22,11 @@
 	}
 }
 
-.block-editor-block-breadcrumb__button.components-button {
-	line-height: $button-size-small;
-	padding: 0;
-	position: relative;
-
-	&:hover:not(:disabled) {
-		text-decoration: underline;
-		box-shadow: none;
-	}
-
-	&:focus {
-		box-shadow: none;
-	}
-
-	&:focus::before {
-		content: "";
-		display: block;
-		position: absolute;
-		border-radius: $radius-block-ui;
-		top: $border-width;
-		right: $border-width;
-		bottom: $border-width;
-		left: $border-width;
-
-		@include button-style__focus();
-	}
-}
-
 .block-editor-block-breadcrumb__current {
 	cursor: default;
 }
 
-.block-editor-block-breadcrumb__button.components-button,
+.block-editor-block-breadcrumb__button.block-editor-block-breadcrumb__button,
 .block-editor-block-breadcrumb__current {
 	color: $gray-900;
 	padding: 0 $grid-unit-10;
Kapture.2024-09-05.at.16.45.51.mp4

What do you folks think? cc @WordPress/gutenberg-design

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ciampo
Copy link
Contributor

ciampo commented Sep 5, 2024

Hi @ciampo For the breadcrumbs it is working fine.

Cool! I actually looked into it and left a comment since I think we have an opportunity to remove almost every style override there.

However, when I apply that to the block mover buttons and remove the height-specific styles from its stylesheet, the height set by the toolbar component has higher specificity and takes priority. Should we override this in the block-mover button styles using $button-size-small, or is there another approach you'd recommend?

Good point. To be honest, the block mover buttons have a lot of custom styles (which, in their turn, override the custom button styles from the toolbar). I'm tempted to leave the style overrides untouched for the scope of this PR, and otherwise consider switching to a vanilla HTML <button /> as an alternative for a low-priority follow-up PR.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Thank you for the cleanup work 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Package] Block editor /packages/block-editor [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants