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

Fix: Button Replace remaining 40px default size violations [Block Editor 3] #65225

Merged
merged 11 commits into from
Sep 24, 2024
Merged

Fix: Button Replace remaining 40px default size violations [Block Editor 3] #65225

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
70ec0f7
Fix: Global Styles component to use 40px default size.
vipul0425 Sep 11, 2024
631a42c
Fix: Block variation picket to use 40px default size.
vipul0425 Sep 11, 2024
36d6a70
Fix: Block variation transform to use 40px default size.
vipul0425 Sep 11, 2024
670cca8
Fix: color gradient dropdown and block appender button to use 40px de…
vipul0425 Sep 11, 2024
913f90c
fix: shadowpanel clear button
vipul0425 Sep 11, 2024
e7b1615
Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/issu…
vipul0425 Sep 16, 2024
bb9545f
fix: Button Block appender issues.
vipul0425 Sep 16, 2024
c452baa
fix: Coverts shadow panel Buttons to normal html buttons.
vipul0425 Sep 16, 2024
c8c0e5d
Update packages/block-library/src/group/editor.scss
vipul0425 Sep 18, 2024
00da47a
Update packages/block-library/src/group/editor.scss
vipul0425 Sep 18, 2024
9d3d8ac
feat: Add tootlip tu shadow button.
vipul0425 Sep 20, 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
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/block-variation-picker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ function BlockVariationPicker( {
{ allowSkip && (
<div className="block-editor-block-variation-picker__skip">
<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.

variation-skip-button-before

  • __next40pxDefaultSize has no effect here because the variation is set to link for this button, which overrides the size with height: auto.

variant="link"
onClick={ () => onSelect() }
>
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/block-variation-transforms/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ function VariationsButtons( {
</VisuallyHidden>
{ variations.map( ( variation ) => (
<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.

Before After
block-variation-transform-before block-variation-transform-after

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this one. I think it should be more like ToggleGroupControl. IE the outer container is 40px, while the inner buttons are 32px (compact size).

Copy link
Contributor

Choose a reason for hiding this comment

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

@jameskoster what about just using a ToggleGroupControl?

Copy link
Contributor

Choose a reason for hiding this comment

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

The visible stroke in the resting state would create additional weight that is probably unwanted in this UI.

We have an issue to explore the ToggleGroupControl design here, which might make it appropriate for use in its vanilla state if implemented.

But until then the simpler approach is probably to just make these buttons compact (32px).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I have updated it to use compact size.

size="compact"
key={ variation.name }
icon={ <BlockIcon icon={ variation.icon } showColors /> }
isPressed={ selectedValue === variation.name }
Expand Down
5 changes: 0 additions & 5 deletions packages/block-editor/src/components/button-block-appender/content.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,6 @@
color: $gray-900;
box-shadow: inset 0 0 0 $border-width $gray-900;

// Needs specificity to override button styles.
&.components-button.components-button {
padding: $grid-unit-15;
}

.is-dark-theme & {
color: $light-gray-placeholder;
box-shadow: inset 0 0 0 $border-width $light-gray-placeholder;
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/button-block-appender/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ function ButtonBlockAppender(

return (
<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.

Before After
button-block-appender-before button-block-appender-after

Copy link
Contributor

Choose a reason for hiding this comment

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

The + icon looks too small. flex-shrink: 0 should fix but there might be a better way?

Related, the empty placeholder that appears adjacent in a Row/Stack has a 46px min-height which causes a mis-match:

Screenshot 2024-09-12 at 16 08 43

It might be good to update that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jameskoster, I have removed the styles that were overriding the component button padding to fix the small icons. Also fixed the empty placeholder height.

image

ref={ mergedInserterButtonRef }
onFocus={ onFocus }
tabIndex={ tabIndex }
Expand Down
6 changes: 1 addition & 5 deletions packages/block-editor/src/components/colors-gradients/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,7 @@ const renderToggle =
};

return (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
{ ...toggleProps }
>
<Button __next40pxDefaultSize { ...toggleProps }>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already taking 40px in height before, so there's no change after setting it to true.

color-panel-button-before-after

<LabeledColorIndicator
colorValue={ colorValue }
label={ label }
Expand Down
6 changes: 1 addition & 5 deletions packages/block-editor/src/components/global-styles/color-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,7 @@ function ColorPanelDropdown( {
};

return (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
{ ...toggleProps }
>
<Button __next40pxDefaultSize { ...toggleProps }>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already taking 40px in height before, so there's no change after setting it to true.

color-panel-button-before-after

<LabeledColorIndicators
indicators={ indicators }
label={ label }
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/global-styles/filters-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,7 @@ export default function FiltersPanel( {
return (
<ItemGroup isBordered isSeparated>
<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.

Before After
filters-button-before filters-button-after

{ ...toggleProps }
>
<LabeledColorIndicator
Expand Down
19 changes: 7 additions & 12 deletions packages/block-editor/src/components/global-styles/shadow-panel-components.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ export function ShadowPopoverContainer( { shadow, onShadowChange, settings } ) {
/>
<div className="block-editor-global-styles__clear-shadow">
<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.

Before After
shadow-clear-button-before shadow-clear-button-after

variant="tertiary"
onClick={ () => onShadowChange( undefined ) }
>
Expand Down Expand Up @@ -88,9 +87,8 @@ export function ShadowIndicator( { type, label, isActive, onSelect, shadow } ) {
'is-active': isActive,
} ) }
render={
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
<button
__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.

Before After
shadow-indicator-before shadow-indicator-after

It looks a bit stretched to me after setting it to 40px. I tried using the small size, but then it appeared a bit too shrunken. Let me know if any adjustments are needed.

Copy link
Contributor

@DaniGuardiola DaniGuardiola Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@WordPress/gutenberg-design ?

Seems to me like we just want normal <button>s (with the original size) here, not sure why use Button if these are fully custom.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the Shadow Palette has used the Button component since it was first implemented: https://github.com/WordPress/gutenberg/pull/46502/files#diff-d3db145a5a2d621586d0b1740cc8b403e58afac79bfe418bf7752fa3af88dc2bR164-R171

I don't know why the Button component was used, but based on this comment, it seems that we were trying to imitate the style of the color palette (CircularOptionPicker.Option component).

In fact, the CircularOptionPicker.Option component is also implemented internally as a Button component and is fully customized (source)

Copy link
Contributor

Choose a reason for hiding this comment

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

For this PR it's probably best to keep those buttons visually looking the same as before, regardless of what component is used. A button element rather than the component may be a fine solution there.

Longer term, we should find a better design for these that's clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to avoid the arbitrary 26px sizing and use of the more standardised options (24, 32, or 40).

Copy link
Member

Choose a reason for hiding this comment

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

I also noticed that wrapping the button with a Tooltip breaks the Composite's keyboard navigation, so we need to wrap the entire Composite.Item with the Tooltip.

☝️ @ciampo Did you know about this quirk? Not immediately sure why that is. The Tooltip embedded in a Button component doesn't break Composite.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I wasn't aware. Maybe we should open a separate issue to at least track this quirk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @t-hamano I have updated <button> as per your suggestion. Let me know if anything else needs to be updated. 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the update. I think it perfectly maintains its functionality and style 👍

Copy link
Member

Choose a reason for hiding this comment

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

Documented the Composite + Tooltip issue at #65615.

className={ clsx(
'block-editor-global-styles__shadow-indicator',
{
Expand All @@ -100,10 +98,11 @@ export function ShadowIndicator( { type, label, isActive, onSelect, shadow } ) {
onClick={ onSelect }
label={ label }
style={ { boxShadow: shadow } }
showTooltip
title={ label }
aria-label={ label }
>
{ isActive && <Icon icon={ check } /> }
</Button>
</button>
}
/>
);
Expand Down Expand Up @@ -143,11 +142,7 @@ function renderShadowToggle() {
};

return (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
{ ...toggleProps }
>
<Button __next40pxDefaultSize { ...toggleProps }>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before After
shadow-toggle-button-before shadow-toggle-button-after

<HStack justify="flex-start">
<Icon
className="block-editor-global-styles__toggle-icon"
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/group/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@
&::after {
content: "";
display: flex;
flex: 1 0 $grid-unit-60;
flex: 1 0 $grid-unit-50;
vipul0425 marked this conversation as resolved.
Show resolved Hide resolved
pointer-events: none;
min-height: $grid-unit-60 - $border-width - $border-width;
min-height: $grid-unit-50 - $border-width - $border-width;
vipul0425 marked this conversation as resolved.
Show resolved Hide resolved
border: $border-width dashed currentColor;
}

Expand Down
Loading