From 10b56ed66c36e2150187a4f0eed84017f68f036a Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Mon, 16 May 2022 13:41:10 -0700 Subject: [PATCH 1/8] 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/8] 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 f02f63b0d0864700c87068a767feb7287269d1c6 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Tue, 17 May 2022 16:18:37 -0700 Subject: [PATCH 3/8] Use DOM APIs to determine if subnav contains current item --- src/NavList/NavList.test.tsx | 2 +- src/NavList/NavList.tsx | 52 +++++++++++++++++++++--------------- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/src/NavList/NavList.test.tsx b/src/NavList/NavList.test.tsx index 3aaac3a5416..6cda27bfb41 100644 --- a/src/NavList/NavList.test.tsx +++ b/src/NavList/NavList.test.tsx @@ -304,6 +304,6 @@ describe('NavList.Item with NavList.SubNav', () => { ) const currentLink = queryByRole('link', {name: 'Current'}) - expect(currentLink).toBeInTheDocument() + expect(currentLink).toBeVisible() }) }) diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index eeeaffd06ed..2f17494a35d 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -48,16 +48,7 @@ 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 => isValidElement(child) && child.props['aria-current'] - ) - - return ( - - {childrenWithoutSubNav} - - ) + return {childrenWithoutSubNav} } return ( @@ -86,25 +77,37 @@ Item.displayName = 'NavList.Item' type ItemWithSubNavProps = { children: React.ReactNode subNav: React.ReactNode - subNavContainsCurrentItem: boolean } -const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: string}>({ +const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: string; isOpen: boolean}>({ buttonId: '', - subNavId: '' + subNavId: '', + isOpen: false }) // TODO: sx prop // TODO: ref prop // TODO: Animate open/close transition -function ItemWithSubNav({children, subNav, subNavContainsCurrentItem}: ItemWithSubNavProps) { +function ItemWithSubNav({children, subNav}: ItemWithSubNavProps) { const buttonId = useSSRSafeId() const subNavId = useSSRSafeId() - // SubNav starts open if current item is in it - const [isOpen, setIsOpen] = React.useState(subNavContainsCurrentItem) + const [isOpen, setIsOpen] = React.useState(false) + const subNavRef = React.useRef(null) + const [containsCurrentItem, setContainsCurrentItem] = React.useState(false) + + React.useLayoutEffect(() => { + if (subNavRef.current) { + // Check if SubNav contains current item + const currentItem = subNavRef.current.querySelector('[aria-current]') + if (currentItem && currentItem.getAttribute('aria-current') !== 'false') { + setContainsCurrentItem(true) + setIsOpen(true) + } + } + }, [subNav]) return ( - + setIsOpen(open => !open)} sx={{ - fontWeight: subNavContainsCurrentItem ? 'bold' : null // Parent item is bold if any of it's sub-items are current + fontWeight: containsCurrentItem ? 'bold' : null // Parent item is bold if any of it's sub-items are current }} > {children} @@ -130,7 +133,7 @@ function ItemWithSubNav({children, subNav, subNavContainsCurrentItem}: ItemWithS - {isOpen ? subNav : null} +
{subNav}
 ) @@ -149,7 +152,7 @@ const SubNavContext = React.createContext<{depth: number}>({depth: 0}) // TODO: ref prop // NOTE: SubNav must be a direct child of an Item const SubNav = ({children}: NavListSubNavProps) => { - const {buttonId, subNavId} = React.useContext(ItemWithSubNavContext) + const {buttonId, subNavId, isOpen} = React.useContext(ItemWithSubNavContext) const {depth} = React.useContext(SubNavContext) if (!buttonId || !subNavId) { @@ -165,7 +168,12 @@ const SubNav = ({children}: NavListSubNavProps) => { return ( - + {children} From 9cdb46bea2190ff16a0bf31ea1fffb3654f8a54b Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Tue, 17 May 2022 16:37:20 -0700 Subject: [PATCH 4/8] Update snapshots --- .../__snapshots__/NavList.test.tsx.snap | 277 +++++++++++++++--- 1 file changed, 243 insertions(+), 34 deletions(-) diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap index 1b9ef3098c1..585b82ef555 100644 --- a/src/NavList/__snapshots__/NavList.test.tsx.snap +++ b/src/NavList/__snapshots__/NavList.test.tsx.snap @@ -838,6 +838,7 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav .c9 { padding: 0; margin: 0; + display: block; } .c7 { @@ -983,6 +984,12 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav --divider-color: transparent; } +.c18:hover:not([aria-disabled]) + .c2, +.c18:focus:not([aria-disabled]) + .c18, +.c18[data-focus-visible-added] + li { + --divider-color: transparent; +} + .c3 { position: relative; display: -webkit-box; @@ -1055,9 +1062,9 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav --divider-color: transparent; } -.c18:hover:not([aria-disabled]) + .c2, -.c18:focus:not([aria-disabled]) + .c18, -.c18[data-focus-visible-added] + li { +.c19:hover:not([aria-disabled]) + .c2, +.c19:focus:not([aria-disabled]) + .c19, +.c19[data-focus-visible-added] + li { --divider-color: transparent; } @@ -1222,34 +1229,36 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav - + @@ -1283,6 +1292,12 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t list-style: none; } +.c9 { + padding: 0; + margin: 0; + display: none; +} + .c5 { display: -webkit-box; display: -webkit-flex; @@ -1317,9 +1332,70 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t padding-bottom: 8px; } -.c9:hover:not([aria-disabled]) + .c2, -.c9:focus:not([aria-disabled]) + .c9, -.c9[data-focus-visible-added] + li { +.c10 { + position: relative; + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + padding-left: 0; + padding-right: 0; + font-size: 14px; + padding-top: 0; + padding-bottom: 0; + line-height: 20px; + min-height: 5px; + margin-left: 8px; + margin-right: 8px; + border-radius: 6px; + -webkit-transition: background 33.333ms linear; + transition: background 33.333ms linear; + color: #24292f; + cursor: pointer; + -webkit-appearance: none; + -moz-appearance: none; + appearance: none; + background: unset; + border: unset; + width: 100%; + font-family: unset; + text-align: unset; + margin-top: unset; + margin-bottom: unset; + font-weight: 600; + background-color: rgba(208,215,222,0.24); +} + +.c10[aria-disabled] { + cursor: not-allowed; +} + +.c10 [data-component="ActionList.Item--DividerContainer"] { + position: relative; +} + +.c10 [data-component="ActionList.Item--DividerContainer"]::before { + content: " "; + display: block; + position: absolute; + width: 100%; + top: -7px; + border: 0 solid; + border-top-width: 0; + border-color: var(--divider-color,transparent); +} + +.c10:not(:first-of-type) { + --divider-color: rgba(208,215,222,0.48); +} + +[data-component="ActionList.Divider"] + .c10 { + --divider-color: transparent !important; +} + +.c10:hover:not([aria-disabled]), +.c10:focus:not([aria-disabled]), +.c10[data-focus-visible-added]:not([aria-disabled]) { --divider-color: transparent; } @@ -1329,10 +1405,15 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t --divider-color: transparent; } -.c11:hover:not([aria-disabled]) + .c2, -.c11:focus:not([aria-disabled]) + .c11, -.c11[data-focus-visible-added] + li { - --divider-color: transparent; +.c10::after { + position: absolute; + top: calc(50% - 12px); + left: -8px; + width: 4px; + height: 24px; + content: ""; + background-color: #0969da; + border-radius: 6px; } .c12:hover:not([aria-disabled]) + .c2, @@ -1365,6 +1446,24 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t --divider-color: transparent; } +.c17:hover:not([aria-disabled]) + .c2, +.c17:focus:not([aria-disabled]) + .c17, +.c17[data-focus-visible-added] + li { + --divider-color: transparent; +} + +.c18:hover:not([aria-disabled]) + .c2, +.c18:focus:not([aria-disabled]) + .c18, +.c18[data-focus-visible-added] + li { + --divider-color: transparent; +} + +.c19:hover:not([aria-disabled]) + .c2, +.c19:focus:not([aria-disabled]) + .c19, +.c19[data-focus-visible-added] + li { + --divider-color: transparent; +} + .c3 { position: relative; display: -webkit-box; @@ -1449,6 +1548,86 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t border-radius: 6px; } +.c11 { + color: #0969da; + -webkit-text-decoration: none; + text-decoration: none; + padding-left: 32px; + padding-right: 8px; + padding-top: 6px; + padding-bottom: 6px; + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + -webkit-box-flex: 1; + -webkit-flex-grow: 1; + -ms-flex-positive: 1; + flex-grow: 1; + border-radius: 6px; + color: inherit; + font-size: 12px; + font-weight: 400; +} + +.c11:hover { + -webkit-text-decoration: underline; + text-decoration: underline; +} + +.c11:is(button) { + display: inline-block; + padding: 0; + font-size: inherit; + white-space: nowrap; + cursor: pointer; + -webkit-user-select: none; + -moz-user-select: none; + -ms-user-select: none; + user-select: none; + background-color: transparent; + border: 0; + -webkit-appearance: none; + -moz-appearance: none; + appearance: none; +} + +.c11:hover { + color: inherit; + -webkit-text-decoration: none; + text-decoration: none; +} + +@media (hover:hover) and (pointer:fine) { + .c10:hover:not([aria-disabled]) { + background-color: rgba(208,215,222,0.32); + color: #24292f; + } + + .c10:focus:not([data-focus-visible-added]) { + background-color: rgba(208,215,222,0.24); + color: #24292f; + outline: none; + } + + .c10[data-focus-visible-added] { + outline: none; + border: 2 solid; + box-shadow: 0 0 0 2px #0969da; + } + + .c10:active:not([aria-disabled]) { + background-color: rgba(208,215,222,0.48); + color: #24292f; + } +} + +@media (forced-colors:active) { + .c10:focus { + outline: solid 1px transparent !important; + } +} + @media (hover:hover) and (pointer:fine) { .c3:hover:not([aria-disabled]) { background-color: rgba(208,215,222,0.32); @@ -1530,6 +1709,36 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t +
+ +
From 01aaff197e43ecbb91bbaa1f14295f43f992a64d Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Tue, 17 May 2022 16:37:44 -0700 Subject: [PATCH 5/8] Remove "not implemented" warnings --- docs/content/NavList.mdx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/content/NavList.mdx b/docs/content/NavList.mdx index 1fb38fd7765..9293821a83f 100644 --- a/docs/content/NavList.mdx +++ b/docs/content/NavList.mdx @@ -174,8 +174,6 @@ If a `NavList.Item` contains a `NavList.SubNav`, the `NavList.Item` will render ### With React Router -Not implemented yet - ```jsx import {Link, useMatch, useResolvedPath} from 'react-router-dom' import {NavList} from '@primer/react' @@ -203,8 +201,6 @@ function App() { ### With Next.js -Not implemented yet - ```jsx import {useRouter} from 'next/router' import Link from 'next/link' From c2efe5811c975c7b43af9eabb1f0110328145b00 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Tue, 17 May 2022 16:40:07 -0700 Subject: [PATCH 6/8] Create dry-feet-attack.md --- .changeset/dry-feet-attack.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/dry-feet-attack.md diff --git a/.changeset/dry-feet-attack.md b/.changeset/dry-feet-attack.md new file mode 100644 index 00000000000..8d41178f0a2 --- /dev/null +++ b/.changeset/dry-feet-attack.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Draft `NavList.Item` now accepts an `as` prop, allowing it to be rendered as a Next.js or React Router link From 517d2615b1c4b4ee8b25be40bc1e9f3733f75ea6 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 19 May 2022 13:24:04 -0700 Subject: [PATCH 7/8] Fix merge issues --- src/NavList/NavList.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index edee6266a92..c86dc471677 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -37,9 +37,8 @@ export type NavListItemProps = { 'aria-current'?: 'page' | 'step' | 'location' | 'date' | 'time' | 'true' | 'false' | boolean } & SxProp - const Item = React.forwardRef( - ({href, 'aria-current': ariaCurrent, children, sx: sxProp = {}}, ref) => { + ({'aria-current': ariaCurrent, children, sx: sxProp = {}, ...props}, ref) => { const {depth} = React.useContext(SubNavContext) // Get SubNav from children @@ -52,7 +51,11 @@ const Item = React.forwardRef( // Render ItemWithSubNav if SubNav is present if (subNav && isValidElement(subNav) && depth < 1) { - return {childrenWithoutSubNav} + return ( + + {childrenWithoutSubNav} + + ) } return ( @@ -68,6 +71,7 @@ const Item = React.forwardRef( }, sxProp )} + {...props} > {children} @@ -124,7 +128,7 @@ function ItemWithSubNav({children, subNav, sx: sxProp = {}}: ItemWithSubNavProps onClick={() => setIsOpen(open => !open)} sx={merge( { - fontWeight: subNavContainsCurrentItem ? 'bold' : null // Parent item is bold if any of it's sub-items are current + fontWeight: containsCurrentItem ? 'bold' : null // Parent item is bold if any of it's sub-items are current }, sxProp )} @@ -183,7 +187,7 @@ const SubNav = ({children, sx: sxProp = {}}: NavListSubNavProps) => { { padding: 0, margin: 0, - display: isOpen ? 'block' : 'none' + display: isOpen ? 'block' : 'none' }, sxProp )} From 070a960bd6e075b9f65796dfd6a829c9b7df947f Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 19 May 2022 13:53:09 -0700 Subject: [PATCH 8/8] Update snapshot --- src/NavList/__snapshots__/NavList.test.tsx.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap index e8b52cfbfb8..87cfd0798f2 100644 --- a/src/NavList/__snapshots__/NavList.test.tsx.snap +++ b/src/NavList/__snapshots__/NavList.test.tsx.snap @@ -1363,7 +1363,7 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t appearance: none; background: unset; border: unset; - width: 100%; + width: calc(100% - 16px); font-family: unset; text-align: unset; margin-top: unset;