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 violation [Block library] #65075

Merged
merged 9 commits into from
Sep 6, 2024
Merged

Fix: Button: Replace remaining 40px default size violation [Block library] #65075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d1e1371
Fix navigation-link block to use button to default 40px size
hbhalodia Sep 4, 2024
d41604c
Fix navigation deleted create new menu button to use 40px default size
hbhalodia Sep 4, 2024
6d2d3df
Fix navigation overlay menu icon button to use 40px default size
hbhalodia Sep 4, 2024
3edc766
Fix the navigation menu delete button to use 40px default size button
hbhalodia Sep 4, 2024
338d4b3
Fix navigation placeholder start empty to use button as default 40px …
hbhalodia Sep 4, 2024
f0febe5
Fix responsive navigation overlay menu open button to use default 40p…
hbhalodia Sep 4, 2024
977b741
Fix navigation responsive close button to use 40px default size
hbhalodia Sep 4, 2024
ecf4b99
Add the !important to override the overlay menu preview style
hbhalodia Sep 6, 2024
7c3d35c
Refactor the comment for the !important style
hbhalodia 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
3 changes: 1 addition & 2 deletions packages/block-library/src/navigation-link/link-ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,7 @@ const LinkUITools = ( { setAddingBlock, focusAddBlockButton } ) => {
return (
<VStack className="link-ui-tools">
<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, while you add navigation-link block and while you add the link.

Before After
navigation-link-before navigation-link-after

ref={ addBlockButtonRef }
icon={ plus }
onClick={ ( e ) => {
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/navigation/edit/deleted-navigation-warning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ function DeletedNavigationWarning( { onCreateNew } ) {
{
button: (
<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 you use navigation ref which is deleted or not present.

Also, this is kind of link which uses button component, but it has some overridden styles, so there would not be any effect on button. Still we have updated component to use default size, in future if specific styles get's removed, we have default 40px being applied.

Before After
navigation-deleted-before navigation-deleted-after

onClick={ onCreateNew }
variant="link"
/>
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,8 +591,7 @@ function Navigation( {
{ isResponsive && (
<>
<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 on the inspector controls, when we add navigation block and we try to set overlay menu icon, refer to below screenshots.

Before After
navigation-overlay-before navigation-overlay-after

Copy link
Member

Choose a reason for hiding this comment

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

We're going to need to preserve this height override. I think we'll need to add an !important here.

gutenberg/packages/block-library/src/navigation/editor.scss

Line 566 in 9523fb4

height: $grid-unit-40 * 2;

Aside: @WordPress/gutenberg-components This is already the second time I'm seeing a height override like this that requires an additional !important (first time was #65068 (comment)). I'm tempted to decrease the specificity of the rule (to &:where( .is-next-40px-default-size )), but I think we shouldn't because:

  • It could cause style regressions in existing usages.
  • The 40px opt-in prop (and thus the CSS class) is temporary.
  • If custom Button height like this is a legitimate design requirement, maybe it should be supported officially?

gutenberg/packages/components/src/button/style.scss

Lines 28 to 30 in 9523fb4

&.is-next-40px-default-size {
height: $button-size-next-default-40px;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're going to need to preserve this height override. I think we'll need to add an !important here.

Thanks for this, would add !important to override the 40px style.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a tricky balance.

I lean on the side of not decreasing the specificity, especially since it's going to be removed soon anyway.

If custom Button height like this is a legitimate design requirement, maybe it should be supported officially?

I'm not sure that adding "one more variant" is a scalable solution.

Seeing how many heavily tweaked instances of Button we're encountering, I would consider (as already discussed in the past) to introduce an "unstyled" variant of Button, which would still benefit from the component's functionality (ie. tooltips, link vs button, focus styles, accessible when disabled, etc) but without any extra cosmetic changes applied (ie. no padding, margin, heighgits, etc).

Alternatively, we could expose a useButton hook which does the same thing, and use if with vanilla <button /> elements.

className={ overlayMenuPreviewClasses }
onClick={ () => {
setOverlayMenuPreview(
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/navigation/edit/navigation-menu-delete-control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ export default function NavigationMenuDeleteControl( { onDelete } ) {
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.

This can be seen, while adding the navigation menu, going to inspector controls advanced tab, and check for Delete Menu button.

Before After
navigation-delete-before navigation-delete-after

className="wp-block-navigation-delete-menu-button"
variant="secondary"
isDestructive
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/navigation/edit/placeholder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ export default function NavigationPlaceholder( {

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

@hbhalodia hbhalodia 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.

I haven't being able to identify this change visually on the site, because the main component, NavigationPlaceholder is exported, but is not used anywhere in entire repository, Ref - https://github.com/search?q=repo%3AWordPress%2Fgutenberg%20export%20default%20function%20NavigationPlaceholder(&type=code,

It is being used here -

gutenberg/packages/block-library/src/navigation/edit/placeholder/index.js

Line 17 in 13af7fd

export default function NavigationPlaceholder( {

@mirka, Can you please verify this.

Thank You,

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it doesn't seem to be used anymore 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for confirmation 👍.

variant="tertiary"
onClick={ onCreateEmpty }
>
Expand Down
6 changes: 2 additions & 4 deletions packages/block-library/src/navigation/edit/responsive-wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ export default function ResponsiveWrapper( {
<>
{ ! isOpen && (
<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, while you add navigation and add the overlay menu icon to mobile or always show.

Note: For this we have some overriding styles on the icon itself, so it would not have before and after change, but we can see that this change have added our 40px change, so in future if overriding styles is removed, we have default 40px of size.

Before After
navigation-responsive-overlay-before navigation-responsive-after

aria-haspopup="true"
aria-label={ hasIcon && __( 'Open menu' ) }
className={ openButtonClasses }
Expand All @@ -102,8 +101,7 @@ export default function ResponsiveWrapper( {
>
<div { ...dialogProps }>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

@hbhalodia hbhalodia 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.

This can be seen, while you add navigation and add the overlay menu icon to mobile or always show, and open the menu by click on overlay icon, you can see the close icon.

Note: For this we have some overriding styles on the icon itself, so it would not have before and after change, but we can see that this change have added our 40px change, so in future if overriding styles is removed, we have default 40px of size.

Before After
Resposnsive-close-before Responsive-close-after

className="wp-block-navigation__responsive-container-close"
aria-label={ hasIcon && __( 'Close menu' ) }
onClick={ () => onToggle( false ) }
Expand Down
6 changes: 5 additions & 1 deletion packages/block-library/src/navigation/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,11 @@ body.editor-styles-wrapper .wp-block-navigation__responsive-container.is-menu-op
width: 100%;
background-color: $gray-100;
padding: 0 $grid-unit-30;
height: $grid-unit-40 * 2;

// Adding !important to override default 40px size.
// Ref - https://github.com/WordPress/gutenberg/pull/65075#discussion_r1746282734
height: $grid-unit-40 * 2 !important;

margin-bottom: $grid-unit-15;

&.open {
Expand Down
Loading