Allow up to 4 levels of nesting for the NavList (#3343)

* allows up to 4 levels of nesting for the NavList * adds changeset * Update src/NavList/NavList.tsx Co-authored-by: Cole Bemis <colebemis@github.com> * updates snapshots --------- Co-authored-by: Cole Bemis <colebemis@github.com>
primer · Jun 15, 2023 · 080f4ce · 080f4ce
1 parent ef13d8f
commit 080f4ce
Show file tree

Hide file tree

Showing 6 changed files with 124 additions and 25 deletions.
diff --git a/.changeset/little-hairs-train.md b/.changeset/little-hairs-train.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': minor
+---
+
+Allows up to 4 levels of nesting in the NavList component.
diff --git 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
 
 </Note>
 
+### With nested sub-items
+
+```jsx live drafts
+<NavList>
+  <NavList.Item href="/branches">Branches</NavList.Item>
+  <NavList.Item href="/tags">Tags</NavList.Item>
+  <NavList.Item>
+    Actions
+    <NavList.SubNav>
+      <NavList.Item href="/actions" aria-current="page">
+        General
+      </NavList.Item>
+      <NavList.Item href="/actions/runners">
+        Runners
+        <NavList.SubNav>
+          <NavList.Item href="/actions/runners/runner-1">Runner 1</NavList.Item>
+          <NavList.Item href="/actions/runners/runner-2">Runner 2</NavList.Item>
+        </NavList.SubNav>
+      </NavList.Item>
+    </NavList.SubNav>
+  </NavList.Item>
+</NavList>
+```
+
+<Note variant="warning">
+
+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.
+
+</Note>
+
 ### With a divider
 
 ```jsx live drafts

diff --git a/src/NavList/NavList.stories.tsx b/src/NavList/NavList.stories.tsx
@@ -47,6 +47,45 @@ export const WithSubItems: Story = () => (
   </PageLayout>
 )
 
+export const WithNestedSubItems: Story = () => (
+  <PageLayout>
+    <PageLayout.Pane position="start">
+      <NavList>
+        <NavList.Item href="#">Item 1</NavList.Item>
+        <NavList.Item href="#">
+          Item 2{/* NOTE: Don't nest SubNavs. For testing purposes only */}
+          <NavList.SubNav>
+            <NavList.Item href="#">
+              Sub item 1
+              <NavList.SubNav>
+                <NavList.Item href="#">
+                  Sub item 1.1
+                  <NavList.SubNav>
+                    <NavList.Item href="#">Sub item 1.1.1</NavList.Item>
+                    <NavList.Item href="#">
+                      Sub item 1.1.2
+                      <NavList.SubNav>
+                        <NavList.Item href="#">Sub item 1.1.2.1</NavList.Item>
+                        <NavList.Item href="#" aria-current="page">
+                          Sub item 1.1.2.2
+                        </NavList.Item>
+                      </NavList.SubNav>
+                    </NavList.Item>
+                  </NavList.SubNav>
+                </NavList.Item>
+                <NavList.Item href="#">Sub item 1.2</NavList.Item>
+              </NavList.SubNav>
+            </NavList.Item>
+            <NavList.Item href="#">Sub item 2</NavList.Item>
+          </NavList.SubNav>
+        </NavList.Item>
+        <NavList.Item href="#">Item 3</NavList.Item>
+      </NavList>
+    </PageLayout.Pane>
+    <PageLayout.Content></PageLayout.Content>
+  </PageLayout>
+)
+
 type ReactRouterLikeLinkProps = {to: string; children: React.ReactNode}
 const ReactRouterLikeLink = React.forwardRef<HTMLAnchorElement, ReactRouterLikeLinkProps>(({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
@@ -249,30 +249,53 @@ 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
       .mockImplementation(() => null)
 
     const {getByRole} = render(
       <NavList>
-        <NavList.Item>
-          Item
+        <NavList.Item href="#">Item 1</NavList.Item>
+        <NavList.Item href="#">
+          Item 2{/* NOTE: Don't nest SubNavs. For testing purposes only */}
           <NavList.SubNav>
-            <NavList.Item>
-              Sub item
-              {/* NOTE: Don't nest SubNavs. For testing purposes only */}
+            <NavList.Item href="#">
+              Sub item 1
               <NavList.SubNav>
-                <NavList.Item href="#">Sub sub item</NavList.Item>
+                <NavList.Item href="#">
+                  Sub item 1.1
+                  <NavList.SubNav>
+                    <NavList.Item href="#">Sub item 1.1.1</NavList.Item>
+                    <NavList.Item href="#">
+                      Sub item 1.1.2
+                      <NavList.SubNav>
+                        <NavList.Item href="#">Sub item 1.1.2.1</NavList.Item>
+                        <NavList.Item href="#">
+                          Sub item 1.1.2.2
+                          <NavList.SubNav>
+                            <NavList.Item href="#" aria-current="page">
+                              Sub item 1.1.2.2.1
+                            </NavList.Item>
+                            <NavList.Item href="#">Sub item 1.1.2.2.2</NavList.Item>
+                          </NavList.SubNav>
+                        </NavList.Item>
+                      </NavList.SubNav>
+                    </NavList.Item>
+                  </NavList.SubNav>
+                </NavList.Item>
+                <NavList.Item href="#">Sub item 1.2</NavList.Item>
               </NavList.SubNav>
             </NavList.Item>
+            <NavList.Item href="#">Sub item 2</NavList.Item>
           </NavList.SubNav>
         </NavList.Item>
+        <NavList.Item href="#">Item 3</NavList.Item>
       </NavList>,
     )
 
-    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
@@ -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 : 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
+  }
+}
+
 // ----------------------------------------------------------------------------
 // NavList
 
@@ -57,9 +65,9 @@ const Item = React.forwardRef<HTMLAnchorElement, NavListItemProps>(
     )
 
     // Render ItemWithSubNav if SubNav is present
-    if (subNav && isValidElement(subNav) && depth < 1) {
+    if (subNav && isValidElement(subNav)) {
       return (
-        <ItemWithSubNav subNav={subNav} sx={sxProp}>
+        <ItemWithSubNav subNav={subNav} depth={depth} sx={sxProp}>
           {childrenWithoutSubNav}
         </ItemWithSubNav>
       )
@@ -70,14 +78,7 @@ const Item = React.forwardRef<HTMLAnchorElement, NavListItemProps>(
         ref={ref}
         aria-current={ariaCurrent}
         active={Boolean(ariaCurrent) && ariaCurrent !== 'false'}
-        sx={merge<SxProp['sx']>(
-          {
-            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<SxProp['sx']>(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<SxProp['sx']>(
             {
+              ...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
@@ -990,7 +990,6 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav
   display: flex;
   padding-left: 8px;
   padding-right: 8px;
-  font-size: 14px;
   padding-top: 6px;
   padding-bottom: 6px;
   line-height: 20px;
@@ -1064,7 +1063,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;
@@ -1458,7 +1457,6 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t
   display: flex;
   padding-left: 8px;
   padding-right: 8px;
-  font-size: 14px;
   padding-top: 6px;
   padding-bottom: 6px;
   line-height: 20px;
@@ -1538,7 +1536,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;