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

Navigation: Upsize back buttons #68157

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Navigation: Upsize back buttons #68157

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15bc1d1
Navigation: Upsize buttons
mirka Dec 19, 2024
0453580
Add changelog
mirka Dec 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
1 change: 1 addition & 0 deletions packages/components/src/navigation/back-button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function UnforwardedNavigationBackButton(
const icon = isRTL() ? chevronRight : chevronLeft;
return (
<MenuBackButtonUI
__next40pxDefaultSize
Copy link
Member Author

Choose a reason for hiding this comment

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

The back buttons should now have 40px height.

Copy link
Member

Choose a reason for hiding this comment

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

They do 👍

Before After
Screenshot 2024-12-20 at 15 49 31 Screenshot 2024-12-20 at 15 49 18

className={ classes }
href={ href }
variant="tertiary"
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/navigation/item/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ export function NavigationItem( props: NavigationItemProps ) {
? restProps
: {
as: Button,
__next40pxDefaultSize:
'as' in restProps ? restProps.as === undefined : true,
Comment on lines +82 to +83
Copy link
Member Author

@mirka mirka Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is just to ensure that this usage is safe for the 40px default size, and won't log deprecation warnings. There should be no visual changes to the navigation items, as their sizes are already overridden.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, we want to apply __next40pxDefaultSize only for Button.

To be extra safe, should we also set it to true when restProps.as is Button? (just in case a consumer sets it explicitly, not sure why)

Copy link
Member Author

@mirka mirka Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good edge case thinking 😄

The reasoning here is that, if there's a Button size violation the console warning will say "wp.components.Button", even if it came from within the NavigationItem black box. That can be confusing to a consumer because for all they know, they just used a NavigationItem and may not even have a single Button instance in their code. But as long as they did an explicit as={ Button }, that Button instance comes from their own code and not from within the NavigationItem black box, and is therefore "discoverable".

href,
onClick: onItemClick,
'aria-current': isActive ? 'page' : undefined,
Expand Down
Loading