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 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 1 commit
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
Next Next commit
Fix: Global Styles component to use 40px default size.
  • Loading branch information
@vipul0425
vipul0425 committed Sep 11, 2024
commit 70ec0f7359a30551c937b2da68db84346b69e8e4
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
10 changes: 2 additions & 8 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,7 +42,6 @@ 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 }
variant="tertiary"
onClick={ () => onShadowChange( undefined ) }
Expand Down Expand Up @@ -89,8 +88,7 @@ export function ShadowIndicator( { type, label, isActive, onSelect, shadow } ) {
} ) }
render={
<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-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 Down Expand Up @@ -143,11 +141,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