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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Sep 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7d7a48f
feat: Adds 40px height in multiple usage warning.
vipul0425 Sep 4, 2024
5a0176f
feat: Adds 4px size in invalid warining.
vipul0425 Sep 4, 2024
77cdf42
feat: Adds the 40px size in block compare component.
vipul0425 Sep 4, 2024
f31118c
feat: Makes the __next40pxDefaultSize true in block mover and breadcr…
vipul0425 Sep 4, 2024
ff95c61
fix: block compare default size.
vipul0425 Sep 4, 2024
607eba2
fix: Invlid warning button size.
vipul0425 Sep 4, 2024
bf1f272
fix: Adds size prop to breadcrumb buttons.
vipul0425 Sep 5, 2024
733e3ce
fix: Updates the snapshot.
vipul0425 Sep 6, 2024
0a6ca4f
refactor: Removes extra button [custom styles.
vipul0425 Sep 6, 2024
8fa351c
Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/issu…
vipul0425 Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions packages/block-editor/src/components/block-breadcrumb/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,8 @@ function BlockBreadcrumb( { rootLabelText } ) {
>
{ hasSelection && (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
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(
Expand All @@ -96,10 +94,8 @@ function BlockBreadcrumb( { rootLabelText } ) {
{ parents.map( ( parentClientId ) => (
<li key={ parentClientId }>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
size="small"
className="block-editor-block-breadcrumb__button"
variant="tertiary"
onClick={ () => selectBlock( parentClientId ) }
>
<BlockTitle
Expand Down
31 changes: 1 addition & 30 deletions packages/block-editor/src/components/block-breadcrumb/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,40 +22,11 @@
}
}

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

@ciampo ciampo Sep 5, 2024 •
edited
Loading

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.

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-small;
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;
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/block-compare/block-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

variant="secondary"
tabIndex="0"
onClick={ action }
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-compare/test/__snapshots__/block-view.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ exports[`BlockView should match snapshot 1`] = `
class="block-editor-block-compare__action"
>
<button
class="components-button is-secondary"
class="components-button is-next-40px-default-size is-secondary"
tabindex="0"
type="button"
>
Expand Down
6 changes: 2 additions & 4 deletions packages/block-editor/src/components/block-edit/multiple-usage-warning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

key="find-original"
variant="secondary"
onClick={ () => selectBlock( originalBlockClientId ) }
>
{ __( 'Find original' ) }
</Button>,
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
key="remove"
variant="secondary"
onClick={ () => onReplace( [] ) }
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/block-list/block-invalid-warning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

key="recover"
onClick={ convert.toRecoveredBlock }
variant="primary"
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/block-mover/button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,7 @@ const BlockMoverButton = forwardRef(
return (
<>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ref={ ref }
className={ clsx(
'block-editor-block-mover-button',
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/block-mover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,7 @@ function BlockMover( {
<BlockDraggable clientIds={ clientIds } fadeWhenDisabled>
{ ( draggableProps ) => (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
icon={ dragHandle }
className="block-editor-block-mover__drag-handle"
label={ __( 'Drag' ) }
Expand Down
Loading