Repository navigation
Admin UI: Add Navigation component and Page navigation slot #79746
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.
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
Changes from 35 commits
4a3a1c0
722e300
19dded9
f5bab03
b918a7f
355ef86
3b1c267
a88f905
8ccc963
23f8f29
d43acbb
10d1746
53158b7
32eba68
e4f4372
158adca
bd33cca
4bdcd73
3e40d75
fe1b0a7
bea588b
16a787f
8a54a4a
1d3ca3a
38687cf
f3632a4
47e31c2
65d92eb
8abe4fc
11912bf
3c7066e
195ae6d
88b5f92
8af048c
554a4c2
38ffb1f
4cfaf48
4259798
085c839
6937f88
96ef59e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { Link, Stack, Text } from '@wordpress/ui'; | ||
| import type { NavigationProps } from './types'; | ||
| import styles from './style.module.css'; | ||
|
|
||
| /** | ||
| * Renders a horizontal list of links for navigating between the sections of a | ||
| * screen. The item whose `href` matches `currentHref` is marked with | ||
| * `aria-current="page"`. | ||
| * | ||
| * Internal to the package: configured through the `Page` `navigation` prop. | ||
| */ | ||
| export const Navigation = ( { | ||
| items, | ||
| currentHref, | ||
| ariaLabel = __( 'Sections' ), | ||
| components, | ||
| className, | ||
| }: NavigationProps ) => { | ||
| if ( ! items.length ) { | ||
| return null; | ||
| } | ||
|
|
||
| const LinkComponent = components?.link; | ||
|
|
||
| if ( process.env.NODE_ENV !== 'production' ) { | ||
| const invalidItem = items.find( | ||
| ( item ) => typeof item.href !== 'string' | ||
| ); | ||
| if ( invalidItem ) { | ||
| throw new Error( | ||
| `Navigation: item "${ invalidItem.label }" is missing an \`href\` prop.` | ||
| ); | ||
| } | ||
|
|
||
| const duplicate = items.find( | ||
| ( item, index ) => | ||
| items.findIndex( ( other ) => other.href === item.href ) !== | ||
| index | ||
| ); | ||
| if ( duplicate ) { | ||
| throw new Error( | ||
| `Navigation: duplicate \`href\` "${ duplicate.href }". Each item must have a unique \`href\` so a single item receives \`aria-current="page"\`.` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <nav aria-label={ ariaLabel } className={ className }> | ||
| { /* | ||
| * Disable reason: The `list` ARIA role is redundant but | ||
| * Safari+VoiceOver won't announce the list otherwise. | ||
| */ | ||
| /* eslint-disable jsx-a11y/no-redundant-roles */ } | ||
| <Stack | ||
| render={ <ul role="list" /> } | ||
| direction="row" | ||
| align="center" | ||
| gap="md" | ||
| className={ styles.list } | ||
| > | ||
| { items.map( ( item ) => ( | ||
| <li key={ item.href } className={ styles.li }> | ||
| <Text | ||
| variant="body-md" | ||
| className={ styles.item } | ||
| render={ | ||
| <Link | ||
| variant="unstyled" | ||
| href={ item.href } | ||
| aria-current={ | ||
| item.href === currentHref | ||
| ? 'page' | ||
| : undefined | ||
| } | ||
| render={ | ||
| LinkComponent && <LinkComponent /> | ||
| } | ||
| /> | ||
| } | ||
| > | ||
| { item.label } | ||
| </Text> | ||
| </li> | ||
| ) ) } | ||
| </Stack> | ||
| { /* eslint-enable jsx-a11y/no-redundant-roles */ } | ||
| </nav> | ||
| ); | ||
| }; | ||
|
|
||
| export default Navigation; | ||
|
ciampo marked this conversation as resolved.
Resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| .list { | ||
| /** | ||
| * Give the scrollport room for the outset focus ring (outline width plus | ||
| * offset), which the overflow clipping would otherwise cut off. The | ||
| * negative margin cancels the padding out of the surrounding layout. | ||
| */ | ||
| --focus-ring-size: calc(2 * var(--wpds-border-width-focus)); | ||
|
|
||
| list-style: none; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This breaks the voice over of a list on Safari, should we add
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, added role="list" on the The jsx-a11y/no-redundant-roles rule flags the explicit role, so it comes with the same disable-reason note that the font library in global-styles-ui already uses for this exact Safari + VoiceOver case. |
||
| padding: var(--focus-ring-size); | ||
| margin: calc(-1 * var(--focus-ring-size)); | ||
| /** | ||
| * Scroll horizontally rather than wrap or shrink when items overflow. | ||
| */ | ||
| overflow-x: auto; | ||
| scroll-padding: var(--focus-ring-size); | ||
| } | ||
|
|
||
| .li { | ||
| flex-shrink: 0; | ||
| } | ||
|
|
||
| .item { | ||
| display: flex; | ||
| align-items: center; | ||
| /** | ||
| * Meet the 44x44px pointer target size of WCAG 2.5.5 Target Size | ||
| * (Enhanced). No design token maps to this accessibility constant. | ||
| */ | ||
| min-block-size: 44px; | ||
|
Comment on lines
+26
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add a 44px min width, too, to fully satisfy the criterion. It's unlikely that a link will ever be that short, but better to make sure, I guess Also, WCAG 2.5.8 is not the correct criterion: That criterion is Target Size (Minimum) and specifies 24×24 CSS pixels (with exceptions); 44×44 is WCAG 2.5.5 Target Size (Enhanced). We should update the comment to 2.5.5
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both done 👍 Labels stay start-aligned, so on a very short item the extra width widens the target without shifting the text. |
||
| min-inline-size: 44px; | ||
| color: var(--wpds-color-foreground-interactive-neutral-weak); | ||
| font-weight: var(--wpds-typography-font-weight-default); | ||
| text-decoration: none; | ||
| } | ||
|
ciampo marked this conversation as resolved.
Resolved
|
||
|
|
||
| .item:hover { | ||
| color: var(--wpds-color-foreground-interactive-neutral-weak-active); | ||
| } | ||
|
|
||
| .item[aria-current="page"] { | ||
| color: var(--wpds-color-foreground-interactive-neutral-weak-active); | ||
| font-weight: var(--wpds-typography-font-weight-emphasis); | ||
| } | ||
|
ciampo marked this conversation as resolved.
Resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| import { render, screen } from '@testing-library/react'; | ||
| import { Navigation } from '..'; | ||
|
|
||
| describe( 'Navigation', () => { | ||
| describe( 'validation', () => { | ||
| it( 'should throw when an item is missing `href`', () => { | ||
| expect( () => | ||
| render( | ||
| <Navigation | ||
| items={ [ | ||
| { label: 'Overview', href: '/overview' }, | ||
| { label: 'Products' } as never, | ||
| ] } | ||
| /> | ||
| ) | ||
| ).toThrow( /item "Products" is missing an `href` prop/ ); | ||
| expect( console ).toHaveErrored(); | ||
| } ); | ||
|
|
||
| it( 'should throw when two items share the same `href`', () => { | ||
| expect( () => | ||
| render( | ||
| <Navigation | ||
| items={ [ | ||
| { label: 'Overview', href: '/overview' }, | ||
| { label: 'Duplicate', href: '/overview' }, | ||
| ] } | ||
| /> | ||
| ) | ||
| ).toThrow( /duplicate `href` "\/overview"/ ); | ||
| expect( console ).toHaveErrored(); | ||
| } ); | ||
|
|
||
| it( 'should not throw when all items have `href`', () => { | ||
| expect( () => | ||
| render( | ||
| <Navigation | ||
| items={ [ | ||
| { label: 'Overview', href: '/overview' }, | ||
| { label: 'Products', href: '/products' }, | ||
| ] } | ||
| /> | ||
| ) | ||
| ).not.toThrow(); | ||
| } ); | ||
|
|
||
| it( 'should not throw when items is empty', () => { | ||
| expect( () => render( <Navigation items={ [] } /> ) ).not.toThrow(); | ||
| } ); | ||
|
|
||
| it( 'should allow an empty string `href`', () => { | ||
| expect( () => | ||
| render( | ||
| <Navigation | ||
| items={ [ | ||
| { label: 'Overview', href: '' }, | ||
| { label: 'Products', href: '/products' }, | ||
| ] } | ||
| currentHref="" | ||
| /> | ||
| ) | ||
| ).not.toThrow(); | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'rendering', () => { | ||
| it( 'should render nothing when items is empty', () => { | ||
| const { container } = render( <Navigation items={ [] } /> ); | ||
| expect( container ).toBeEmptyDOMElement(); | ||
| } ); | ||
|
|
||
| it( 'should render inside a nav with a default accessible label', () => { | ||
| render( | ||
| <Navigation | ||
| items={ [ { label: 'Overview', href: '/overview' } ] } | ||
| /> | ||
| ); | ||
|
|
||
| expect( | ||
| screen.getByRole( 'navigation', { | ||
| name: 'Sections', | ||
| } ) | ||
| ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'should use a custom `ariaLabel` when provided', () => { | ||
| render( | ||
| <Navigation | ||
| items={ [ { label: 'Overview', href: '/overview' } ] } | ||
| ariaLabel="Analytics sections" | ||
| /> | ||
| ); | ||
|
|
||
| expect( | ||
| screen.getByRole( 'navigation', { | ||
| name: 'Analytics sections', | ||
| } ) | ||
| ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'should render each item as a link with its `href`', () => { | ||
| render( | ||
| <Navigation | ||
| items={ [ | ||
| { label: 'Overview', href: '/overview' }, | ||
| { label: 'Products', href: '/products' }, | ||
| ] } | ||
| /> | ||
| ); | ||
|
|
||
| const links = screen.getAllByRole( 'link' ); | ||
| expect( links ).toHaveLength( 2 ); | ||
| expect( links[ 0 ] ).toHaveTextContent( 'Overview' ); | ||
| expect( links[ 0 ] ).toHaveAttribute( 'href', '/overview' ); | ||
| expect( links[ 1 ] ).toHaveTextContent( 'Products' ); | ||
| expect( links[ 1 ] ).toHaveAttribute( 'href', '/products' ); | ||
| } ); | ||
|
|
||
| it( 'should mark the item matching `currentHref` with aria-current="page"', () => { | ||
| render( | ||
| <Navigation | ||
| items={ [ | ||
| { label: 'Overview', href: '/overview' }, | ||
| { label: 'Products', href: '/products' }, | ||
| ] } | ||
| currentHref="/overview" | ||
| /> | ||
| ); | ||
|
|
||
| expect( | ||
| screen.getByRole( 'link', { name: 'Overview' } ) | ||
| ).toHaveAttribute( 'aria-current', 'page' ); | ||
| expect( | ||
| screen.getByRole( 'link', { name: 'Products' } ) | ||
| ).not.toHaveAttribute( 'aria-current' ); | ||
| } ); | ||
|
|
||
| it( 'should mark an empty string `href` as current when `currentHref` is empty', () => { | ||
| render( | ||
| <Navigation | ||
| items={ [ | ||
| { label: 'Overview', href: '' }, | ||
| { label: 'Products', href: '/products' }, | ||
| ] } | ||
| currentHref="" | ||
| /> | ||
| ); | ||
|
|
||
| // Disable reason: A link with an empty `href` doesn't surface in | ||
| // the accessibility three via `getByRole` with `name`. In practice | ||
| // this would likely not be something we would recommend doing, but | ||
| // this test verifies the expected behavior of current link testing. | ||
| // eslint-disable-next-line testing-library/no-node-access | ||
| const overviewLink = screen.getByText( 'Overview' ).closest( 'a' ); | ||
| expect( overviewLink ).toHaveAttribute( 'href', '' ); | ||
| expect( overviewLink ).toHaveAttribute( 'aria-current', 'page' ); | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'components', () => { | ||
| it( 'should render items with the provided link component', () => { | ||
| const CustomLink = ( { | ||
| children, | ||
| ...props | ||
| }: React.AnchorHTMLAttributes< HTMLAnchorElement > ) => ( | ||
| <a data-custom="true" { ...props }> | ||
| { children } | ||
| </a> | ||
| ); | ||
|
|
||
| render( | ||
| <Navigation | ||
| items={ [ | ||
| { label: 'Overview', href: '/overview' }, | ||
| { label: 'Products', href: '/products' }, | ||
| ] } | ||
| currentHref="/overview" | ||
| components={ { link: CustomLink } } | ||
| /> | ||
| ); | ||
|
|
||
| const link = screen.getByRole( 'link', { name: 'Overview' } ); | ||
| expect( link ).toHaveAttribute( 'data-custom', 'true' ); | ||
| expect( link ).toHaveAttribute( 'href', '/overview' ); | ||
| expect( link ).toHaveAttribute( 'aria-current', 'page' ); | ||
| expect( | ||
| screen.getByRole( 'link', { name: 'Products' } ) | ||
| ).toHaveAttribute( 'data-custom', 'true' ); | ||
| } ); | ||
| } ); | ||
| } ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { type AnchorHTMLAttributes, type ComponentType } from 'react'; | ||
|
|
||
| export interface NavigationItem { | ||
| /** | ||
| * The label text for the navigation item. | ||
| */ | ||
| label: string; | ||
|
|
||
| /** | ||
| * The URL the item links to. | ||
| */ | ||
| href: string; | ||
| } | ||
|
|
||
| export interface NavigationComponents { | ||
| /** | ||
| * Component rendered in place of the default `<a>` for each item, e.g. a | ||
| * client-side router link. It receives standard anchor attributes | ||
| * (`href`, `aria-current`, `className`, `children`) and must forward | ||
| * them to the element it renders. | ||
| * | ||
| * @default 'a' | ||
| */ | ||
| link?: ComponentType< AnchorHTMLAttributes< HTMLAnchorElement > >; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With type RouteLinkProps = {
href: string;
children: React.ReactNode;
className?: string;
};
const RouteLink = ( { href, ...props }: RouteLinkProps ) => (
<Link to={ href } { ...props } />
);The current test and story hide this problem by typing their fake anchor with the same optional-
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is a good catch, and it was a little tricky to navigate, between how exactly to make it required (e.g.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe
I guess one approach here is to split this into a public data-only config (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good call 👍 I split the internal and public-facing types in 6937f88. |
||
| } | ||
|
|
||
| export interface NavigationProps { | ||
|
ciampo marked this conversation as resolved.
Resolved
|
||
| /** | ||
| * The navigation items to display. | ||
| */ | ||
| items: readonly NavigationItem[]; | ||
|
|
||
| /** | ||
| * The `href` of the current item. The matching item is rendered with | ||
| * `aria-current="page"` and a distinct visual treatment. | ||
| */ | ||
| currentHref?: string; | ||
|
|
||
| /** | ||
| * Accessible label for the navigation landmark. Give each navigation on a | ||
| * screen a unique label, and omit the word "navigation" since the landmark | ||
| * role already conveys it. | ||
| * | ||
| * @default 'Sections' | ||
| */ | ||
| ariaLabel?: string; | ||
|
|
||
| /** | ||
| * Overrides for the elements the navigation renders. | ||
| */ | ||
| components?: NavigationComponents; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question (cc @aduth ): should
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likely yes, and in fact my initial proposal in #77040 included exactly that ("
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated this in 4cfaf48 so that |
||
|
|
||
| /** | ||
| * Optional class name for the root element. | ||
| */ | ||
| className?: string; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.