From 10b56ed66c36e2150187a4f0eed84017f68f036a Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Mon, 16 May 2022 13:41:10 -0700 Subject: [PATCH 1/5] Add as prop to NavList.Item --- src/NavList/NavList.stories.tsx | 59 ++++++++++++++++++++++++++++++++- src/NavList/NavList.test.tsx | 50 ++++++++++++++++++++++++++++ src/NavList/NavList.tsx | 8 ++--- 3 files changed, 112 insertions(+), 5 deletions(-) diff --git a/src/NavList/NavList.stories.tsx b/src/NavList/NavList.stories.tsx index ca2e667f1f5..62d6125d5c5 100644 --- a/src/NavList/NavList.stories.tsx +++ b/src/NavList/NavList.stories.tsx @@ -26,7 +26,7 @@ export const Simple: Story = () => ( ) -export const SubItems: Story = () => ( +export const WithSubItems: Story = () => ( @@ -47,4 +47,61 @@ export const SubItems: Story = () => ( ) +type ReactRouterLikeLinkProps = {to: string; children: React.ReactNode} +const ReactRouterLikeLink = React.forwardRef(({to, ...props}, ref) => { + // eslint-disable-next-line jsx-a11y/anchor-has-content + return +}) + +export const WithReactRouterLink = () => ( + + + + + Item 1 + + + Item 2 + + + Item 3 + + + + + +) + +type NextJSLinkProps = {href: string; children: React.ReactNode} + +const NextJSLikeLink = React.forwardRef( + ({href, children}, ref): React.ReactElement => { + const child = React.Children.only(children) + const childProps = { + ref, + href + } + return <>{React.isValidElement(child) ? React.cloneElement(child, childProps) : null} + } +) + +export const WithNextJSLink = () => ( + + + + + Item 1 + + + Item 2 + + + Item 3 + + + + + +) + export default meta diff --git a/src/NavList/NavList.test.tsx b/src/NavList/NavList.test.tsx index 8acb971d40c..4085953db55 100644 --- a/src/NavList/NavList.test.tsx +++ b/src/NavList/NavList.test.tsx @@ -60,6 +60,56 @@ describe('NavList.Item', () => { expect(homeLink).toHaveAttribute('aria-current', 'page') expect(aboutLink).not.toHaveAttribute('aria-current') }) + + it('is compatiable with React-Router-like link components', () => { + type ReactRouterLikeLinkProps = {to: string; children: React.ReactNode} + + const ReactRouterLikeLink = React.forwardRef(({to, ...props}, ref) => { + // eslint-disable-next-line jsx-a11y/anchor-has-content + return + }) + + const {getByRole} = render( + + + React Router link + + + ) + + const link = getByRole('link', {name: 'React Router link'}) + + expect(link).toHaveAttribute('aria-current', 'page') + expect(link).toHaveAttribute('href', '/') + }) + + it('is compatible with NextJS-like link components', () => { + type NextJSLinkProps = {href: string; children: React.ReactNode} + + const NextJSLikeLink = React.forwardRef( + ({href, children}, ref): React.ReactElement => { + const child = React.Children.only(children) + const childProps = { + ref, + href + } + return <>{React.isValidElement(child) ? React.cloneElement(child, childProps) : null} + } + ) + + const {getByRole} = render( + + + NextJS link + + + ) + + const link = getByRole('link', {name: 'NextJS link'}) + + expect(link).toHaveAttribute('href', '/') + expect(link).toHaveAttribute('aria-current', 'page') + }) }) describe('NavList.Item with NavList.SubNav', () => { diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index 5cea60fb008..eeeaffd06ed 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -1,4 +1,5 @@ import {ChevronDownIcon} from '@primer/octicons-react' +import {ForwardRefComponent as PolymorphicForwardRefComponent} from '@radix-ui/react-polymorphic' import {useSSRSafeId} from '@react-aria/ssr' import React, {isValidElement} from 'react' import {ActionList} from '../ActionList' @@ -33,9 +34,8 @@ export type NavListItemProps = { } // TODO: sx prop -// TODO: as prop const Item = React.forwardRef( - ({href, 'aria-current': ariaCurrent, children}, ref) => { + ({'aria-current': ariaCurrent, children, ...props}, ref) => { const {depth} = React.useContext(SubNavContext) // Get SubNav from children @@ -63,7 +63,6 @@ const Item = React.forwardRef( return ( ( fontSize: depth > 0 ? 0 : null, // Reduce font size of sub-items fontWeight: depth > 0 ? 'normal' : null // Sub-items don't get bolded }} + {...props} > {children} ) } -) +) as PolymorphicForwardRefComponent<'a', NavListItemProps> Item.displayName = 'NavList.Item' From 91b408f737087443f81f597085353bd787a19572 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Mon, 16 May 2022 14:18:28 -0700 Subject: [PATCH 2/5] Add test case for react router link with subnav --- src/NavList/NavList.test.tsx | 69 +++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 20 deletions(-) diff --git a/src/NavList/NavList.test.tsx b/src/NavList/NavList.test.tsx index 4085953db55..3aaac3a5416 100644 --- a/src/NavList/NavList.test.tsx +++ b/src/NavList/NavList.test.tsx @@ -3,6 +3,26 @@ import React from 'react' import {ThemeProvider, SSRProvider} from '..' import {NavList} from './NavList' +type ReactRouterLikeLinkProps = {to: string; children: React.ReactNode} + +const ReactRouterLikeLink = React.forwardRef(({to, ...props}, ref) => { + // eslint-disable-next-line jsx-a11y/anchor-has-content + return +}) + +type NextJSLinkProps = {href: string; children: React.ReactNode} + +const NextJSLikeLink = React.forwardRef( + ({href, children}, ref): React.ReactElement => { + const child = React.Children.only(children) + const childProps = { + ref, + href + } + return <>{React.isValidElement(child) ? React.cloneElement(child, childProps) : null} + } +) + describe('NavList', () => { it('renders a simple list', () => { const {container} = render( @@ -62,13 +82,6 @@ describe('NavList.Item', () => { }) it('is compatiable with React-Router-like link components', () => { - type ReactRouterLikeLinkProps = {to: string; children: React.ReactNode} - - const ReactRouterLikeLink = React.forwardRef(({to, ...props}, ref) => { - // eslint-disable-next-line jsx-a11y/anchor-has-content - return - }) - const {getByRole} = render( @@ -84,19 +97,6 @@ describe('NavList.Item', () => { }) it('is compatible with NextJS-like link components', () => { - type NextJSLinkProps = {href: string; children: React.ReactNode} - - const NextJSLikeLink = React.forwardRef( - ({href, children}, ref): React.ReactElement => { - const child = React.Children.only(children) - const childProps = { - ref, - href - } - return <>{React.isValidElement(child) ? React.cloneElement(child, childProps) : null} - } - ) - const {getByRole} = render( @@ -277,4 +277,33 @@ describe('NavList.Item with NavList.SubNav', () => { expect(consoleSpy).toHaveBeenCalled() }) + + it('is compatiable with React-Router-like link components', () => { + function NavLink({href, children}: {href: string; children: React.ReactNode}) { + // In a real app, you'd check if the href matches the url of the current page. For testing purposes, we'll use the text of the link to determine if it's current + const isCurrent = children === 'Current' + return ( + + {children} + + ) + } + + const {queryByRole} = render( + + Item 1 + + Item 2 + + Current + Sub item 2 + + + Item 3 + + ) + + const currentLink = queryByRole('link', {name: 'Current'}) + expect(currentLink).toBeInTheDocument() + }) }) From 78b0feb5fd1baccb7231993f964f0bfb24ae2370 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 17 May 2022 12:28:42 +0200 Subject: [PATCH 3/5] go one level deeper --- src/NavList/NavList.tsx | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index eeeaffd06ed..fbf8cdab9bb 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -50,7 +50,20 @@ const Item = React.forwardRef( if (subNav && isValidElement(subNav) && depth < 1) { // Search SubNav children for current Item const currentItem = React.Children.toArray(subNav.props.children).find( - child => isValidElement(child) && child.props['aria-current'] + (child) => { + if (!isValidElement(child)) return false + + // when direct child is SubNav.Item + if (child.type === Item) return child.props['aria-current'] + + // when direct child isn't SubNav.Item, + // it's probably a NextJSLikeLink, go one level deeper + const wrappedItem = child.props.children + if (typeof wrappedItem === 'object') return wrappedItem.props['aria-current'] + + // we don't recognise this API usage + return false + } ) return ( From 6a43b0e0e807c86e09a3c5c0526a147a8cb05485 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 17 May 2022 12:34:21 +0200 Subject: [PATCH 4/5] run prettier for CI --- src/NavList/NavList.tsx | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index fbf8cdab9bb..1037bcd602c 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -49,22 +49,20 @@ const Item = React.forwardRef( // Render ItemWithSubNav if SubNav is present if (subNav && isValidElement(subNav) && depth < 1) { // Search SubNav children for current Item - const currentItem = React.Children.toArray(subNav.props.children).find( - (child) => { - if (!isValidElement(child)) return false + const currentItem = React.Children.toArray(subNav.props.children).find(child => { + if (!isValidElement(child)) return false - // when direct child is SubNav.Item - if (child.type === Item) return child.props['aria-current'] + // when direct child is SubNav.Item + if (child.type === Item) return child.props['aria-current'] - // when direct child isn't SubNav.Item, - // it's probably a NextJSLikeLink, go one level deeper - const wrappedItem = child.props.children - if (typeof wrappedItem === 'object') return wrappedItem.props['aria-current'] + // when direct child isn't SubNav.Item, + // it's probably a NextJSLikeLink, go one level deeper + const wrappedItem = child.props.children + if (typeof wrappedItem === 'object') return wrappedItem.props['aria-current'] - // we don't recognise this API usage - return false - } - ) + // we don't recognise this API usage + return false + }) return ( From d05d4d04e3db0bb5685daee6f7d518ed483cd70c Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 17 May 2022 15:25:44 +0200 Subject: [PATCH 5/5] Add warning for missing aria-current --- src/NavList/NavList.tsx | 46 ++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index 1037bcd602c..40e2c3fd205 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -5,6 +5,7 @@ import React, {isValidElement} from 'react' import {ActionList} from '../ActionList' import Box from '../Box' import StyledOcticon from '../StyledOcticon' +import {useProvidedRefOrCreate} from '../hooks' // ---------------------------------------------------------------------------- // NavList @@ -13,10 +14,34 @@ export type NavListProps = { children: React.ReactNode } & React.ComponentProps<'nav'> +const useWarningForMissingAriaCurrent = containerRef => { + React.useEffect( + function checkForAriaCurrentOnMount() { + const ariaCurrentEl = containerRef.current?.querySelector('[aria-current]') + + if (!ariaCurrentEl) + throw new Error(` +aria-current not found on NavList.Item + +To create an accessible navigation, it is required to add aria-current to the current anchor element. + +See https://primer.style/react/NavList#accessibilty for more info + +If you are extending NavList.Item to create a navigation element, make sure you are passing aria-current at the site of usage. +See https://primer.style/react/NavList#extend for more info. + `) + }, + [containerRef] + ) +} + // TODO: sx prop const Root = React.forwardRef(({children, ...props}, ref) => { + const ensureRef = useProvidedRefOrCreate(ref) + useWarningForMissingAriaCurrent(ensureRef) + return ( -