feat(Nav): performance improvements (#55)

* feat(Nav): WIP performance

* clean up code, move sorting to codefence

* update tests to reflect removal of sorting logic

* update test, snapshot, fix active detection

Author: kmcfaul

src/components/Masthead.astro

@@ -16,7 +16,7 @@ import { PageToggle } from './PageToggle'
 <PFMasthead>
   <MastheadMain>
     <MastheadToggle>
-      <PageToggle client:idle />
+      <PageToggle client:idle transition:persist />
     </MastheadToggle>
     <MastheadBrand>
       <MastheadLogo component="a" href="/">

src/components/NavEntry.tsx

@@ -6,9 +6,7 @@ export interface TextContentEntry {
   data: {
     id: string
     section: string
-    tab?: string
   }
-  collection: string
 }
 
 interface NavEntryProps {

src/components/NavSection.tsx

@@ -14,30 +14,10 @@ export const NavSection = ({
   sectionId,
   activeItem,
 }: NavSectionProps) => {
-  const isExpanded = window.location.pathname.includes(sectionId)
+  const isExpanded = window.location.pathname.includes(kebabCase(sectionId))
+  const isActive = entries.some((entry) => entry.id === activeItem)
 
-  const sortedNavEntries = entries.sort((a, b) =>
-    a.data.id.localeCompare(b.data.id),
-  )
-
-  const isActive = sortedNavEntries.some((entry) => entry.id === activeItem)
-
-  let navItems = sortedNavEntries
-  if (sectionId === 'components' || sectionId === 'layouts') {
-    // only display unique entry.data.id in the nav list if the section is components
-    navItems = [
-      ...sortedNavEntries
-        .reduce((map, entry) => {
-          if (!map.has(entry.data.id)) {
-            map.set(entry.data.id, entry)
-          }
-          return map
-        }, new Map())
-        .values(),
-    ]
-  }
-
-  const items = navItems.map((entry) => (
+  const items = entries.map((entry) => (
     <NavEntry
       key={entry.id}
       entry={entry}

src/components/Navigation.astro

@@ -2,6 +2,7 @@
 import { getCollection } from 'astro:content'
 
 import { Navigation as ReactNav } from './Navigation.tsx'
+import { TextContentEntry } from './NavEntry.tsx'
 
 import { content } from '../content'
 
@@ -13,12 +14,56 @@ const collections = await Promise.all(
   ),
 )
 
-const navEntries = collections.flat()
+const navDataRaw = collections.flat();
+const uniqueSections = new Set(navDataRaw.map((entry) => entry.data.section));
+const navData: Record<string, TextContentEntry[]> = {};
+
+const [orderedSections, unorderedSections] = Array.from(uniqueSections).reduce(
+    (acc, section) => {
+      if (!config.navSectionOrder) {
+        acc[1].push(section)
+        return acc
+      }
+
+      const index = config.navSectionOrder.indexOf(section)
+      if (index > -1) {
+        acc[0][index] = section
+      } else {
+        acc[1].push(section)
+      }
+      return acc
+    },
+    [[], []] as [string[], string[]],
+  )
+
+const sortedSections = [...orderedSections, ...unorderedSections.sort()]
+sortedSections.map((section) => {
+  const entries = navDataRaw
+    .filter((entry) => entry.data.section === section)
+    .map(entry => ({ id: entry.id, data: { id: entry.data.id, section }} as TextContentEntry))
+
+  const sortedEntries = entries.sort((a, b) =>
+    a.data.id.localeCompare(b.data.id),
+  )
+
+  let navEntries = sortedEntries
+  if (section === 'components' || section === 'layouts') {
+    // only display unique entry.data.id in the nav list if the section is components
+    navEntries = [
+      ...sortedEntries
+        .reduce((map, entry) => {
+          if (!map.has(entry.data.id)) {
+            map.set(entry.data.id, entry)
+          }
+          return map
+        }, new Map())
+        .values(),
+    ]
+  }
+
+  navData[section] = navEntries;
+})
+
 ---
 
-<ReactNav
-  client:only="react"
-  navEntries={navEntries}
-  navSectionOrder={config.navSectionOrder}
-  transition:animate="fade"
-/>
+<ReactNav client:only="react" navData={navData} transition:animate="fade" />

src/components/Navigation.tsx

@@ -4,19 +4,17 @@ import { NavSection } from './NavSection'
 import { type TextContentEntry } from './NavEntry'
 
 interface NavigationProps {
-  navEntries: TextContentEntry[]
-  navSectionOrder?: string[]
+  navData: Record<string, TextContentEntry[]>
 }
 
 export const Navigation: React.FunctionComponent<NavigationProps> = ({
-  navEntries,
-  navSectionOrder,
+  navData,
 }: NavigationProps) => {
   const [activeItem, setActiveItem] = useState('')
 
   useEffect(() => {
     // TODO: Needs an alternate solution because of /tab in the path
-    setActiveItem(window.location.pathname.split('/').reverse()[0]) 
+    setActiveItem(window.location.pathname.split('/').reverse()[0])
   }, [])
 
   const onNavSelect = (
@@ -26,48 +24,20 @@ export const Navigation: React.FunctionComponent<NavigationProps> = ({
     setActiveItem(selectedItem.itemId.toString())
   }
 
-  const uniqueSections = Array.from(
-    new Set(navEntries.map((entry) => entry.data.section)),
-  )
-
-  // We want to list any ordered sections first, followed by any unordered sections sorted alphabetically
-  const [orderedSections, unorderedSections] = uniqueSections.reduce(
-    (acc, section) => {
-      if (!navSectionOrder) {
-        acc[1].push(section)
-        return acc
-      }
-
-      const index = navSectionOrder.indexOf(section)
-      if (index > -1) {
-        acc[0][index] = section
-      } else {
-        acc[1].push(section)
-      }
-      return acc
-    },
-    [[], []] as [string[], string[]],
-  )
-  const sortedSections = [...orderedSections, ...unorderedSections.sort()]
-
-  const navSections = sortedSections.map((section) => {
-    const entries = navEntries.filter((entry) => entry.data.section === section)
-
-    return (
-      <NavSection
-        key={section}
-        entries={entries}
-        sectionId={section}
-        activeItem={activeItem}
-      />
-    )
-  })
-
   return (
     // Can possibly add back PageSidebar wrapper when https://github.com/patternfly/patternfly/issues/7377 goes in
     <PageSidebarBody id="page-sidebar-body">
       <Nav onSelect={onNavSelect}>
-        <NavList>{navSections}</NavList>
+        <NavList>
+          {Object.entries(navData).map(([key, value], index) => (
+            <NavSection
+              key={index}
+              entries={value}
+              sectionId={key}
+              activeItem={activeItem}
+            />
+          ))}
+        </NavList>
       </Nav>
     </PageSidebarBody>
   )

src/components/__tests__/NavEntry.test.tsx

@@ -1,36 +1,36 @@
-
-import { render, screen } from '@testing-library/react';
-import { NavEntry, TextContentEntry } from '../NavEntry';
+import { render, screen } from '@testing-library/react'
+import { NavEntry, TextContentEntry } from '../NavEntry'
 
 const mockEntry: TextContentEntry = {
   id: 'entry1',
   data: { id: 'Entry1', section: 'section1' },
-  collection: 'textContent',
-};
+}
 
 describe('NavEntry', () => {
   it('renders without crashing', () => {
-    render(<NavEntry entry={mockEntry} isActive={false} />);
-    expect(screen.getByText('Entry1')).toBeInTheDocument();
-  });
+    render(<NavEntry entry={mockEntry} isActive={false} />)
+    expect(screen.getByText('Entry1')).toBeInTheDocument()
+  })
 
   it('renders the correct link', () => {
-    render(<NavEntry entry={mockEntry} isActive={false} />);
-    expect(screen.getByRole('link')).toHaveAttribute('href', '/section1/entry1');
-  });
+    render(<NavEntry entry={mockEntry} isActive={false} />)
+    expect(screen.getByRole('link')).toHaveAttribute('href', '/section1/entry1')
+  })
 
   it('marks the entry as active if isActive is true', () => {
-    render(<NavEntry entry={mockEntry} isActive={true} />);
-    expect(screen.getByRole('link')).toHaveClass('pf-m-current');
-  });
+    render(<NavEntry entry={mockEntry} isActive={true} />)
+    expect(screen.getByRole('link')).toHaveClass('pf-m-current')
+  })
 
   it('does not mark the entry as active if isActive is false', () => {
-    render(<NavEntry entry={mockEntry} isActive={false} />);
-    expect(screen.getByRole('link')).not.toHaveClass('pf-m-current');
-  });
+    render(<NavEntry entry={mockEntry} isActive={false} />)
+    expect(screen.getByRole('link')).not.toHaveClass('pf-m-current')
+  })
 
   it('matches snapshot', () => {
-    const { asFragment } = render(<NavEntry entry={mockEntry} isActive={false} />);
-    expect(asFragment()).toMatchSnapshot();
-  });
-});
+    const { asFragment } = render(
+      <NavEntry entry={mockEntry} isActive={false} />,
+    )
+    expect(asFragment()).toMatchSnapshot()
+  })
+})

src/components/__tests__/NavSection.test.tsx

@@ -6,35 +6,14 @@ const mockEntries: TextContentEntry[] = [
   {
     id: 'entry1',
     data: { id: 'Entry1', section: 'section1' },
-    collection: 'textContent',
   },
   {
     id: 'entry2',
     data: { id: 'Entry2', section: 'Section1' },
-    collection: 'textContent',
   },
   {
     id: 'entry3',
     data: { id: 'Entry3', section: 'Section1' },
-    collection: 'textContent',
-  },
-]
-
-const dupedEntries: TextContentEntry[] = [
-  {
-    id: 'entry1',
-    data: { id: 'Entry1', section: 'components' },
-    collection: 'react',
-  },
-  {
-    id: 'entry2',
-    data: { id: 'Entry1', section: 'components' },
-    collection: 'react',
-  },
-  {
-    id: 'entry3',
-    data: { id: 'Entry2', section: 'components' },
-    collection: 'react',
   },
 ]
 
@@ -159,45 +138,3 @@ it('matches snapshot', () => {
   )
   expect(asFragment()).toMatchSnapshot()
 })
-
-it('dedupes and renders correct number of entries for components section', () => {
-  Object.defineProperty(window, 'location', {
-    value: {
-      pathname: '/foo/components',
-    },
-    writable: true,
-  })
-
-  render(
-    <NavSection
-      entries={dupedEntries}
-      sectionId="components"
-      activeItem="entry1"
-    />,
-  )
-
-  expect(screen.getAllByRole('listitem')).toHaveLength(3)
-  expect(screen.getByRole('link', { name: 'Entry1' })).toBeInTheDocument()
-  expect(screen.getByRole('link', { name: 'Entry2' })).toBeInTheDocument()
-})
-
-it('dedupes and renders correct number of entries for layouts section', () => {
-  Object.defineProperty(window, 'location', {
-    value: {
-      pathname: '/foo/layouts',
-    },
-    writable: true,
-  })
-
-  render(
-    <NavSection
-      entries={dupedEntries}
-      sectionId="layouts"
-      activeItem="entry1"
-    />,
-  )
-
-  expect(screen.getAllByRole('listitem')).toHaveLength(3)
-  expect(screen.getByRole('link', { name: 'Entry1' })).toBeInTheDocument()
-  expect(screen.getByRole('link', { name: 'Entry2' })).toBeInTheDocument()
-})