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 all 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
61 changes: 28 additions & 33 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 @@ -11,6 +11,7 @@ import {
FlexItem,
Dropdown,
Composite,
Tooltip,
} from '@wordpress/components';
import { useMemo } from '@wordpress/element';
import { shadow as shadowIcon, Icon, check } from '@wordpress/icons';
Expand Down Expand Up @@ -42,8 +43,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 @@ -80,32 +80,31 @@ export function ShadowPresets( { presets, activeShadow, onSelect } ) {

export function ShadowIndicator( { type, label, isActive, onSelect, shadow } ) {
return (
<Composite.Item
role="option"
aria-label={ label }
aria-selected={ isActive }
className={ clsx( 'block-editor-global-styles__shadow__item', {
'is-active': isActive,
} ) }
render={
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
className={ clsx(
'block-editor-global-styles__shadow-indicator',
{
unset: type === 'unset',
}
) }
onClick={ onSelect }
label={ label }
style={ { boxShadow: shadow } }
showTooltip
>
{ isActive && <Icon icon={ check } /> }
</Button>
}
/>
<Tooltip text={ label }>
<Composite.Item
role="option"
aria-label={ label }
aria-selected={ isActive }
className={ clsx( 'block-editor-global-styles__shadow__item', {
'is-active': isActive,
} ) }
render={
<button
className={ clsx(
'block-editor-global-styles__shadow-indicator',
{
unset: type === 'unset',
}
) }
onClick={ onSelect }
style={ { boxShadow: shadow } }
aria-label={ label }
>
{ isActive && <Icon icon={ check } /> }
</button>
}
/>
</Tooltip>
);
}

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
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/global-styles/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,15 @@

// These styles are similar to the color palette.
.block-editor-global-styles__shadow-indicator {
appearance: none;
background: none;
color: $gray-800;
border: $gray-200 $border-width solid;
border-radius: $radius-small;
cursor: pointer;
display: inline-flex;
align-items: center;

padding: 0;

height: $button-size-small + 2 * $border-width;
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 $button-size-next-default-40px;
pointer-events: none;
min-height: $grid-unit-60 - $border-width - $border-width;
min-height: $button-size-next-default-40px - $border-width - $border-width;
border: $border-width dashed currentColor;
}

Expand Down
Loading