From 26b35b0be77433569af9cdcb44afb38fe09e3aee Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Tue, 30 May 2023 10:50:42 -0400 Subject: [PATCH 1/4] allows up to 4 levels of nesting for the NavList --- docs/content/NavList.mdx | 30 ++++++++++++++ src/NavList/NavList.stories.tsx | 39 +++++++++++++++++++ src/NavList/NavList.test.tsx | 39 +++++++++++++++---- src/NavList/NavList.tsx | 30 +++++++------- .../__snapshots__/NavList.test.tsx.snap | 14 +++---- 5 files changed, 123 insertions(+), 29 deletions(-) diff --git a/docs/content/NavList.mdx b/docs/content/NavList.mdx index 3e12b73f7d6..fc205fc6f57 100644 --- a/docs/content/NavList.mdx +++ b/docs/content/NavList.mdx @@ -161,6 +161,36 @@ If a `NavList.Item` contains a `NavList.SubNav`, the `NavList.Item` will render +### With nested sub-items + +```jsx live drafts + + Branches + Tags + + Actions + + + General + + + Runners + + Runner 1 + Runner 2 + + + + + +``` + + + +We only allow for up to 4 levels of nested subnavs. If more than 4 levels is required, it's a good sign that the navigation design needs to be rethought. + + + ### With a divider ```jsx live drafts diff --git a/src/NavList/NavList.stories.tsx b/src/NavList/NavList.stories.tsx index f643302c1f8..9af62a887ee 100644 --- a/src/NavList/NavList.stories.tsx +++ b/src/NavList/NavList.stories.tsx @@ -64,6 +64,45 @@ export const WithSubItems: Story = () => ( ) +export const WithNestedSubItems: Story = () => ( + + + + Item 1 + + Item 2{/* NOTE: Don't nest SubNavs. For testing purposes only */} + + + Sub item 1 + + + Sub item 1.1 + + Sub item 1.1.1 + + Sub item 1.1.2 + + Sub item 1.1.2.1 + + Sub item 1.1.2.2 + + + + + + Sub item 1.2 + + + Sub item 2 + + + Item 3 + + + + +) + type ReactRouterLikeLinkProps = {to: string; children: React.ReactNode} const ReactRouterLikeLink = React.forwardRef(({to, ...props}, ref) => { // eslint-disable-next-line jsx-a11y/anchor-has-content diff --git a/src/NavList/NavList.test.tsx b/src/NavList/NavList.test.tsx index 08fe311ae60..ee4fa595285 100644 --- a/src/NavList/NavList.test.tsx +++ b/src/NavList/NavList.test.tsx @@ -249,7 +249,7 @@ describe('NavList.Item with NavList.SubNav', () => { expect(container).toMatchSnapshot() }) - it('prevents multiple levels of nested SubNavs', () => { + it('prevents more than 4 levels of nested SubNavs', () => { const consoleSpy = jest .spyOn(console, 'error') // Suppress error message in test output @@ -257,22 +257,45 @@ describe('NavList.Item with NavList.SubNav', () => { const {getByRole} = render( - - Item + Item 1 + + Item 2{/* NOTE: Don't nest SubNavs. For testing purposes only */} - - Sub item - {/* NOTE: Don't nest SubNavs. For testing purposes only */} + + Sub item 1 - Sub sub item + + Sub item 1.1 + + Sub item 1.1.1 + + Sub item 1.1.2 + + Sub item 1.1.2.1 + + Sub item 1.1.2.2 + + + Sub item 1.1.2.2.1 + + Sub item 1.1.2.2.2 + + + + + + + Sub item 1.2 + Sub item 2 + Item 3 , ) - const item = getByRole('button', {name: 'Item'}) + const item = getByRole('button', {name: 'Item 2'}) fireEvent.click(item) expect(consoleSpy).toHaveBeenCalled() diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index 80550abc989..5b39aa88b2e 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -15,6 +15,14 @@ import {defaultSxProp} from '../utils/defaultSxProp' import {useId} from '../hooks/useId' import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' +const getSubnavStyles = (depth: number) => { + return { + paddingLeft: depth > 0 ? depth + 2 : 0, // Indent sub-items + fontSize: depth > 0 ? 0 : null, // Reduce font size of sub-items + fontWeight: depth > 0 ? 'normal' : null, // Sub-items don't get bolded + } +} + // ---------------------------------------------------------------------------- // NavList @@ -57,9 +65,9 @@ const Item = React.forwardRef( ) // Render ItemWithSubNav if SubNav is present - if (subNav && isValidElement(subNav) && depth < 1) { + if (subNav && isValidElement(subNav)) { return ( - + {childrenWithoutSubNav} ) @@ -70,14 +78,7 @@ const Item = React.forwardRef( ref={ref} aria-current={ariaCurrent} active={Boolean(ariaCurrent) && ariaCurrent !== 'false'} - sx={merge( - { - paddingLeft: depth > 0 ? 5 : null, // Indent sub-items - fontSize: depth > 0 ? 0 : null, // Reduce font size of sub-items - fontWeight: depth > 0 ? 'normal' : null, // Sub-items don't get bolded - }, - sxProp, - )} + sx={merge(getSubnavStyles(depth), sxProp)} {...props} > {children} @@ -94,6 +95,7 @@ Item.displayName = 'NavList.Item' type ItemWithSubNavProps = { children: React.ReactNode subNav: React.ReactNode + depth: number } & SxProp const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: string; isOpen: boolean}>({ @@ -104,7 +106,7 @@ const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: s // TODO: ref prop // TODO: Animate open/close transition -function ItemWithSubNav({children, subNav, sx: sxProp = defaultSxProp}: ItemWithSubNavProps) { +function ItemWithSubNav({children, subNav, depth, sx: sxProp = defaultSxProp}: ItemWithSubNavProps) { const buttonId = useId() const subNavId = useId() const [isOpen, setIsOpen] = React.useState(false) @@ -135,6 +137,7 @@ function ItemWithSubNav({children, subNav, sx: sxProp = defaultSxProp}: ItemWith onClick={() => setIsOpen(open => !open)} sx={merge( { + ...getSubnavStyles(depth), fontWeight: containsCurrentItem ? 'bold' : null, // Parent item is bold if any of it's sub-items are current }, sxProp, @@ -178,9 +181,10 @@ const SubNav = ({children, sx: sxProp = defaultSxProp}: NavListSubNavProps) => { console.error('NavList.SubNav must be a child of a NavList.Item') } - if (depth > 0) { + if (depth > 3) { + // TODO: write a more informative error message that directs people to rethink their IA // eslint-disable-next-line no-console - console.error('NavList.SubNav only supports one level of nesting') + console.error('NavList.SubNav only supports four levels of nesting') return null } diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap index 443240545e4..2ae0bf364c5 100644 --- a/src/NavList/__snapshots__/NavList.test.tsx.snap +++ b/src/NavList/__snapshots__/NavList.test.tsx.snap @@ -193,7 +193,7 @@ exports[`NavList renders a simple list 1`] = ` color: #0969da; -webkit-text-decoration: none; text-decoration: none; - padding-left: 8px; + padding-left: 0; padding-right: 8px; padding-top: 6px; padding-bottom: 6px; @@ -597,7 +597,7 @@ exports[`NavList renders with groups 1`] = ` color: #0969da; -webkit-text-decoration: none; text-decoration: none; - padding-left: 8px; + padding-left: 0; padding-right: 8px; padding-top: 6px; padding-bottom: 6px; @@ -1003,9 +1003,8 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav display: -webkit-flex; display: -ms-flexbox; display: flex; - padding-left: 8px; + padding-left: 0; padding-right: 8px; - font-size: 14px; padding-top: 6px; padding-bottom: 6px; line-height: 20px; @@ -1079,7 +1078,7 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav color: #0969da; -webkit-text-decoration: none; text-decoration: none; - padding-left: 32px; + padding-left: 16px; padding-right: 8px; padding-top: 6px; padding-bottom: 6px; @@ -1478,9 +1477,8 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t display: -webkit-flex; display: -ms-flexbox; display: flex; - padding-left: 8px; + padding-left: 0; padding-right: 8px; - font-size: 14px; padding-top: 6px; padding-bottom: 6px; line-height: 20px; @@ -1560,7 +1558,7 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t color: #0969da; -webkit-text-decoration: none; text-decoration: none; - padding-left: 32px; + padding-left: 16px; padding-right: 8px; padding-top: 6px; padding-bottom: 6px; From c1c4949778c3b8e2eec81e56099b1116a9ef3b36 Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Tue, 30 May 2023 10:55:14 -0400 Subject: [PATCH 2/4] adds changeset --- .changeset/little-hairs-train.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/little-hairs-train.md diff --git a/.changeset/little-hairs-train.md b/.changeset/little-hairs-train.md new file mode 100644 index 00000000000..1725b26816b --- /dev/null +++ b/.changeset/little-hairs-train.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +Allows up to 4 levels of nesting in the NavList component. From 0b3b28ead6e22b8eea430c2fded029bf949afcdf Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Mon, 5 Jun 2023 16:56:07 -0400 Subject: [PATCH 3/4] Update src/NavList/NavList.tsx Co-authored-by: Cole Bemis --- src/NavList/NavList.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index 5b39aa88b2e..086ef324715 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -17,7 +17,7 @@ import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' const getSubnavStyles = (depth: number) => { return { - paddingLeft: depth > 0 ? depth + 2 : 0, // Indent sub-items + paddingLeft: depth > 0 ? depth + 2 : null, // Indent sub-items fontSize: depth > 0 ? 0 : null, // Reduce font size of sub-items fontWeight: depth > 0 ? 'normal' : null, // Sub-items don't get bolded } From 218092de5353bf48c5dd33fb2925b75a8e6de6a4 Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Tue, 6 Jun 2023 09:22:29 -0400 Subject: [PATCH 4/4] updates snapshots --- src/NavList/__snapshots__/NavList.test.tsx.snap | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap index 2ae0bf364c5..2fc235e8e21 100644 --- a/src/NavList/__snapshots__/NavList.test.tsx.snap +++ b/src/NavList/__snapshots__/NavList.test.tsx.snap @@ -193,7 +193,7 @@ exports[`NavList renders a simple list 1`] = ` color: #0969da; -webkit-text-decoration: none; text-decoration: none; - padding-left: 0; + padding-left: 8px; padding-right: 8px; padding-top: 6px; padding-bottom: 6px; @@ -597,7 +597,7 @@ exports[`NavList renders with groups 1`] = ` color: #0969da; -webkit-text-decoration: none; text-decoration: none; - padding-left: 0; + padding-left: 8px; padding-right: 8px; padding-top: 6px; padding-bottom: 6px; @@ -1003,7 +1003,7 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav display: -webkit-flex; display: -ms-flexbox; display: flex; - padding-left: 0; + padding-left: 8px; padding-right: 8px; padding-top: 6px; padding-bottom: 6px; @@ -1477,7 +1477,7 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t display: -webkit-flex; display: -ms-flexbox; display: flex; - padding-left: 0; + padding-left: 8px; padding-right: 8px; padding-top: 6px; padding-bottom: 6px;