simplify Table of Contents (github#28833)

* simplify table-of-contents * key * fix rendering tests * Not so much top margin * Map topic ToC links are spans * use p tag * Back to span for the article title * Update comment to match markup * remove hack * use h2 instead * fix tests * fix use of key * use regular className instead Co-authored-by: Robert Sese <[email protected]>
mohammadzolfali · Jul 14, 2022 · 37d3bde · 37d3bde
1 parent 7b81003
commit 37d3bde
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 33 deletions.
diff --git a/components/landing/TableOfContents.tsx b/components/landing/TableOfContents.tsx
@@ -1,9 +1,7 @@
-import { useRouter } from 'next/router'
 import cx from 'classnames'
 
 import { ActionList } from '@primer/react'
 import { Link } from 'components/Link'
-import { BumpLink } from 'components/ui/BumpLink'
 import type { TocItem } from 'components/context/ProductLandingContext'
 
 type Props = {
@@ -12,22 +10,43 @@ type Props = {
 }
 export const TableOfContents = (props: Props) => {
   const { items, variant = 'expanded' } = props
-  const router = useRouter()
+
+  const actionItems = (items || []).filter((item) => typeof item !== 'undefined')
 
   return (
     <ul
       data-testid="table-of-contents"
-      className={cx(variant === 'compact' ? 'list-style-outside pl-2' : 'list-style-none')}
+      className={cx(variant === 'compact' ? 'list-style-outside pl-2' : '')}
     >
-      <ActionList variant="full">
-        {(items || [])
-          .filter((item) => typeof item !== 'undefined')
-          .map((item) => {
-            const { fullPath: href, title, intro, childTocItems } = item
-            const isActive = router.pathname === href
-            return variant === 'compact' ? (
+      {variant === 'expanded' &&
+        actionItems.map((item) => {
+          const { fullPath: href, title, intro } = item
+
+          return (
+            <li
+              key={href}
+              data-testid="expanded-item"
+              className="pt-4 pb-3 f4 d-list-item width-full list-style-none border-bottom"
+            >
+              <h2 className="py-1 h4">
+                <Link href={href} className="color-fg-accent">
+                  {title}
+                </Link>
+              </h2>
+              {intro && (
+                <p className="f4 color-fg-muted" dangerouslySetInnerHTML={{ __html: intro }} />
+              )}
+            </li>
+          )
+        })}
+
+      {variant === 'compact' && (
+        <ActionList>
+          {actionItems.map((item) => {
+            const { fullPath: href, title, childTocItems } = item
+            return (
               <ActionList.Item key={href}>
-                <div className="f4 d-list-item width-full list-style-none">
+                <li className="f4 d-list-item width-full list-style-none">
                   <Link className="d-block width-full text-underline" href={href}>
                     {title}
                   </Link>
@@ -54,24 +73,12 @@ export const TableOfContents = (props: Props) => {
                       })}
                     </ul>
                   )}
-                </div>
-              </ActionList.Item>
-            ) : (
-              <ActionList.Item key={href} className={cx('border-bottom')}>
-                <div className={cx('mt-2', isActive && 'color-fg-muted')}>
-                  <BumpLink as={Link} href={href} title={<h2 className="py-1 h4">{title}</h2>}>
-                    {intro && (
-                      <p
-                        className="f4 color-fg-muted"
-                        dangerouslySetInnerHTML={{ __html: intro }}
-                      />
-                    )}
-                  </BumpLink>
-                </div>
+                </li>
               </ActionList.Item>
             )
           })}
-      </ActionList>
+        </ActionList>
+      )}
     </ul>
   )
 }
diff --git a/components/landing/TocLanding.tsx b/components/landing/TocLanding.tsx
@@ -78,7 +78,7 @@ export const TocLanding = () => {
 
           <div className="border-bottom border-xl-0 pb-4 mb-5 pb-xl-2 mb-xl-2" />
 
-          <div className={variant === 'expanded' ? 'mt-7' : 'mt-2'}>
+          <div className={variant === 'expanded' ? 'mt-5' : 'mt-2'}>
             {featuredLinks.gettingStarted && featuredLinks.popular && (
               <div className="pb-8 container-xl">
                 <div className="gutter gutter-xl-spacious clearfix">

diff --git a/tests/rendering/server.js b/tests/rendering/server.js
@@ -718,13 +718,13 @@ describe('server', () => {
       expect($('[data-testid=table-of-contents] ul li a').length).toBeGreaterThan(5)
     })
 
-    test('map topic renders with h2 links to articles', async () => {
+    test('map topic renders with links to articles', async () => {
       const $ = await getDOM(
         '/en/get-started/importing-your-projects-to-github/importing-source-code-to-github'
       )
       expect(
         $(
-          'a[href="/en/get-started/importing-your-projects-to-github/importing-source-code-to-github/about-github-importer"] h2'
+          'li h2 a[href="/en/get-started/importing-your-projects-to-github/importing-source-code-to-github/about-github-importer"]'
         ).length
       ).toBe(1)
     })
@@ -733,15 +733,18 @@ describe('server', () => {
       const $ = await getDOM(
         '/en/get-started/importing-your-projects-to-github/importing-source-code-to-github'
       )
-      const $bumpLinks = $('[data-testid=bump-link]')
-      expect($bumpLinks.length).toBeGreaterThan(3)
+      const $links = $('[data-testid=expanded-item]')
+      expect($links.length).toBeGreaterThan(3)
     })
 
     test('map topic intros are parsed', async () => {
       const $ = await getDOM(
         '/en/get-started/importing-your-projects-to-github/importing-source-code-to-github'
       )
-      const $intro = $('[data-testid=bump-link][href*="source-code-migration-tools"] > p')
+      const $parent = $('[data-testid=expanded-item] a[href*="source-code-migration-tools"]')
+        .parent()
+        .parent()
+      const $intro = $('p', $parent)
       expect($intro.length).toBe(1)
       expect($intro.html()).toContain('You can use external tools to move your projects to GitHub')
     })