From 7ecafa279d934770651e76dab914e65f3501853e Mon Sep 17 00:00:00 2001 From: Grace Park Date: Tue, 28 Mar 2023 09:58:13 -0700 Subject: [PATCH] Updating sidebar to TreeView (#35455) --- .../sidebar/ProductCollapsibleSection.tsx | 139 ++++------ .../RestCollapsibleSection.module.scss | 14 + components/sidebar/RestCollapsibleSection.tsx | 248 ++++++++---------- components/sidebar/SidebarHomepage.tsx | 69 +++-- components/sidebar/SidebarProduct.module.scss | 13 - components/sidebar/SidebarProduct.tsx | 167 ++++++------ tests/browser/browser.js | 136 +++++++++- .../playwright-rendering.spec.ts | 4 +- tests/rendering-fixtures/sidebar.js | 16 +- tests/rendering/sidebar.js | 121 +-------- 10 files changed, 443 insertions(+), 484 deletions(-) create mode 100644 components/sidebar/RestCollapsibleSection.module.scss delete mode 100644 components/sidebar/SidebarProduct.module.scss diff --git a/components/sidebar/ProductCollapsibleSection.tsx b/components/sidebar/ProductCollapsibleSection.tsx index c1f5198fcd39..94f6c2e19e81 100644 --- a/components/sidebar/ProductCollapsibleSection.tsx +++ b/components/sidebar/ProductCollapsibleSection.tsx @@ -1,122 +1,77 @@ import cx from 'classnames' -import { useState, SyntheticEvent } from 'react' -import { ChevronDownIcon } from '@primer/octicons-react' -import { ActionList } from '@primer/react' +import { TreeView } from '@primer/react' import { Link } from 'components/Link' import { ProductTreeNode } from 'components/context/MainContext' import { EventType, sendEvent } from 'src/events/browser' -import styles from './SidebarProduct.module.scss' type SectionProps = { routePath: string page: ProductTreeNode title: string - defaultOpen: boolean } export const ProductCollapsibleSection = (props: SectionProps) => { - const { routePath, defaultOpen, title, page } = props - const [isOpen, setIsOpen] = useState(defaultOpen) - - const onToggle = (e: SyntheticEvent) => { - const newIsOpen = (e.target as HTMLDetailsElement).open - setIsOpen(newIsOpen) - sendEvent({ - type: EventType.navigate, - navigate_label: `details ${newIsOpen ? 'open' : 'close'}: ${title}`, - }) - } - + const { routePath, page } = props // The lowest level page link displayed in the tree const renderTerminalPageLink = (page: ProductTreeNode) => { const title = page.shortTitle || page.title - const isCurrent = routePath === page.href + return ( - - { + sendEvent({ + type: EventType.navigate, + navigate_label: `product page navigate to: ${page.href}`, + }) + }} > {title} - - + + ) } return ( -
- -
-
- {title} -
- - - -
- - - { + <> + {/* */} + {page.childPages[0]?.documentType === 'mapTopic' ? ( <> - {/* */} - {page.childPages[0]?.documentType === 'mapTopic' ? ( - - ) : page.childPages[0]?.documentType === 'article' ? ( -
- - {page.childPages.map(renderTerminalPageLink)} - -
- ) : null} + return ( +
+ + {childTitle} + + {childPage.childPages.map((cp) => { + return renderTerminalPageLink(cp) + })} + + +
+ ) + })} - } -
+ ) : page.childPages[0]?.documentType === 'article' ? ( +
+ {page.childPages.map((cp) => { + return renderTerminalPageLink(cp) + })} +
+ ) : null} + ) } diff --git a/components/sidebar/RestCollapsibleSection.module.scss b/components/sidebar/RestCollapsibleSection.module.scss new file mode 100644 index 000000000000..b22d8723c7fe --- /dev/null +++ b/components/sidebar/RestCollapsibleSection.module.scss @@ -0,0 +1,14 @@ +.operationWidth { + li div div span { + white-space: normal !important; + padding: 2px; + } +} + +.toggleHover { + li div div { + &:hover { + background-color: transparent !important; + } + } +} diff --git a/components/sidebar/RestCollapsibleSection.tsx b/components/sidebar/RestCollapsibleSection.tsx index c68d294ce13d..095b4365fb76 100644 --- a/components/sidebar/RestCollapsibleSection.tsx +++ b/components/sidebar/RestCollapsibleSection.tsx @@ -1,45 +1,33 @@ import { useRouter } from 'next/router' import cx from 'classnames' -import { useState, useEffect, SyntheticEvent, ReactElement } from 'react' -import { ChevronDownIcon } from '@primer/octicons-react' -import { ActionList } from '@primer/react' +import { useState, useEffect } from 'react' +import { TreeView } from '@primer/react' import { Link } from 'components/Link' import { ProductTreeNode } from 'components/context/MainContext' import { EventType, sendEvent } from 'src/events/browser' import { useAutomatedPageContext } from 'components/context/AutomatedPageContext' import type { MiniTocItem } from 'components/context/ArticleContext' -import styles from './SidebarProduct.module.scss' + +import styles from './RestCollapsibleSection.module.scss' + +let prevTarget: object | undefined type SectionProps = { routePath: string page: ProductTreeNode title: string - defaultOpen: boolean isStandaloneCategory: boolean } -type ConditionalLinkTypes = { - condition: boolean - wrapper: Function - children: ReactElement -} - export const RestCollapsibleSection = (props: SectionProps) => { const router = useRouter() - const { routePath, defaultOpen, title, page, isStandaloneCategory } = props - const [isOpen, setIsOpen] = useState(defaultOpen) + const { routePath, title, page, isStandaloneCategory } = props const [currentAnchor, setCurrentAnchor] = useState('') const [visibleAnchor, setVisibleAnchor] = useState('') - - const onToggle = (e: SyntheticEvent) => { - const newIsOpen = (e.target as HTMLDetailsElement).open - setIsOpen(newIsOpen) - sendEvent({ - type: EventType.navigate, - navigate_label: `details ${newIsOpen ? 'open' : 'close'}: ${title}`, - }) - } + const isActive = routePath.includes(page.href + '/') || routePath === page.href + const [standAloneExpanded, setStandAloneExpanded] = useState(isActive) + const [mapTopicExpanded, setMapTopicExpanded] = useState(isActive) const miniTocItems = router.query.productId === 'rest' || @@ -100,138 +88,126 @@ export const RestCollapsibleSection = (props: SectionProps) => { } } }, [miniTocItems]) - // This wrapper solves the issue of having standalone categories not - // link to the new page. We want standalone categories to have links - // just like maptopics/subcategories. - const ConditionalLinkWrapper = ({ condition, wrapper, children }: ConditionalLinkTypes) => - condition ? wrapper(children) : children + + useEffect(() => { + setMapTopicExpanded(true) + }, [router.query.subcategory]) const renderRestAnchorLink = (miniTocItem: MiniTocItem) => { const miniTocAnchor = miniTocItem.contents.href const title = miniTocItem.contents.title - const isCurrent = visibleAnchor === miniTocAnchor + const isAnchorCurrent = visibleAnchor === miniTocAnchor + return ( - setVisibleAnchor(miniTocAnchor)} + href={miniTocAnchor} + className={cx(styles.operationWidth, 'color-fg-default no-underline')} > - setVisibleAnchor(miniTocAnchor)} - href={miniTocAnchor} + {title} - - + + ) } return ( -
- - ( - - {children} - - )} + // This is where a category has no subcategory +
+ {isStandaloneCategory ? ( + -
-
- {title} -
- - - -
- -
- - { - <> - {/* */} - - - } -
+ + ) : ( +
+ )} + + + + ) + })} + + + )} + ) } diff --git a/components/sidebar/SidebarHomepage.tsx b/components/sidebar/SidebarHomepage.tsx index 86bf0316c011..e64e3dba51e7 100644 --- a/components/sidebar/SidebarHomepage.tsx +++ b/components/sidebar/SidebarHomepage.tsx @@ -1,6 +1,6 @@ import { useRouter } from 'next/router' import { LinkExternalIcon } from '@primer/octicons-react' -import { ActionList } from '@primer/react' +import { TreeView } from '@primer/react' import { useVersion } from 'components/hooks/useVersion' import { useMainContext } from 'components/context/MainContext' @@ -12,44 +12,37 @@ export const SidebarHomepage = () => { const { activeProducts, isFPT } = useMainContext() return ( - + {product.name} + {product.external && ( + + + + )} + + + ) + })} + + ) } diff --git a/components/sidebar/SidebarProduct.module.scss b/components/sidebar/SidebarProduct.module.scss deleted file mode 100644 index 5696dd25683a..000000000000 --- a/components/sidebar/SidebarProduct.module.scss +++ /dev/null @@ -1,13 +0,0 @@ -.sidebarArticle::before { - content: ""; - position: absolute; - left: calc(1.5rem + 2px); - height: 100%; - border-left: 1px solid var(--color-fg-default); - width: 1px; - top: 0; -} - -.sidebarArticleActive::before { - border-left-width: 2px; -} diff --git a/components/sidebar/SidebarProduct.tsx b/components/sidebar/SidebarProduct.tsx index d4daac4f93e0..70817d0d6fe7 100644 --- a/components/sidebar/SidebarProduct.tsx +++ b/components/sidebar/SidebarProduct.tsx @@ -1,5 +1,6 @@ import { useRouter } from 'next/router' import { useEffect, useRef } from 'react' +import { TreeView } from '@primer/react' import cx from 'classnames' import { useMainContext } from 'components/context/MainContext' @@ -10,13 +11,13 @@ import { ProductCollapsibleSection } from './ProductCollapsibleSection' export const SidebarProduct = () => { const router = useRouter() - const sidebarRef = useRef(null) + const sidebarRef = useRef(null) const { currentProduct, currentProductTree } = useMainContext() const { t } = useTranslation(['products']) const isRestPage = currentProduct && currentProduct.id === 'rest' useEffect(() => { - const activeArticle = document.querySelector('[data-is-current-page=true]') + const activeArticle = document.querySelector('[aria-expanded=true]') // Setting to the top doesn't give enough context of surrounding categories activeArticle?.scrollIntoView({ block: 'center' }) // scrollIntoView affects some articles that are very low in the sidebar @@ -32,48 +33,50 @@ export const SidebarProduct = () => { // remove query string and hash const routePath = `/${router.locale}${router.asPath.split('?')[0].split('#')[0]}` - const hasExactCategory = !!currentProductTree?.childPages.find(({ href }) => - routePath.includes(href) - ) - const productSection = () => ( -
  • -
      +
      + {currentProductTree && currentProductTree.childPages.map((childPage, i) => { const isStandaloneCategory = childPage.documentType === 'article' - const childTitle = childPage.shortTitle || childPage.title const isActive = routePath.includes(childPage.href + '/') || routePath === childPage.href - const defaultOpen = hasExactCategory ? isActive : false + return ( -
    • +
      {isStandaloneCategory ? ( - - {childTitle} - + + {childTitle} + + ) : ( - + + {childTitle} + + + + )} -
      • +
      ) })} -
    -
    • + + ) const restSection = () => { @@ -91,89 +94,79 @@ export const SidebarProduct = () => { ) return ( <> -
  • -
      +
      + {conceptualPages.map((childPage, i) => { - const isStandaloneCategory = childPage.documentType === 'article' const childTitle = childPage.shortTitle || childPage.title const isActive = routePath.includes(childPage.href + '/') || routePath === childPage.href - const defaultOpen = hasExactCategory ? isActive : false return ( -
    • +
      {childPage.href.includes('quickstart') ? ( - + + {childTitle} + + + ) : ( + {childTitle} - - ) : ( - + + + + )} -
      • +
      ) })} -
    -
    • + +
    - + {t('rest.reference.api_reference')}
    -
  • -
      - {restPages.map((childPage, i) => { - const isStandaloneCategory = childPage.documentType === 'article' - const childTitle = childPage.shortTitle || childPage.title - const isActive = - routePath.includes(childPage.href + '/') || routePath === childPage.href - const defaultOpen = hasExactCategory ? isActive : false - return ( -
    • - -
      • - ) - })} -
    -
    • + + {restPages.map((childPage, i) => { + const isStandaloneCategory = childPage.documentType === 'article' + const childTitle = childPage.shortTitle || childPage.title + + return ( + + ) + })} + ) } return ( -
      +
      {isRestPage ? restSection() : productSection()} -
    + ) } diff --git a/tests/browser/browser.js b/tests/browser/browser.js index 693d3ce3fe9b..4bd5a08dd1be 100644 --- a/tests/browser/browser.js +++ b/tests/browser/browser.js @@ -1,8 +1,17 @@ +import fs from 'fs' +import path from 'path' import { jest } from '@jest/globals' import { oldestSupported } from '../../lib/enterprise-server-releases.js' -jest.useFakeTimers({ legacyFakeTimers: true }) +import { getDOM } from '../helpers/e2etest.js' +import frontmatter from '../../lib/read-frontmatter.js' +import getApplicableVersions from '../../lib/get-applicable-versions.js' +import { allVersions } from '../../lib/all-versions.js' +import renderContent from '../../lib/render-content/index.js' +import shortVersionsMiddleware from '../../middleware/contextualizers/short-versions.js' +jest.useFakeTimers({ legacyFakeTimers: true }) +const req = {} /* global page, browser */ describe('homepage', () => { jest.setTimeout(60 * 1000) @@ -509,3 +518,128 @@ describe('iframe pages', () => { ).toBeTruthy() }) }) + +describe('REST sidebar', () => { + req.context = { + allVersions, + currentLanguage: 'en', + } + + it('Check REST categories and subcategories are rendering', async () => { + // Get the titles from the content/rest directory to match the titles on the page + const contentFiles = getCatAndSubCat('content/rest') + const contentCheck = await createContentCheckDirectory(contentFiles) + + for (const version in allVersions) { + // Get MapTopic level categories/subcategories for each version on /rest page + const url = `/en/${version}/rest` + const $ = await getDOM(url) + + const categories = [] + $('[data-testid=sidebar] [data-testid=rest-category]').each((i, el) => { + categories[i] = $(el).text() + }) + const browserUrl = `http://localhost:4000${url}` + await page.goto(browserUrl) + await page.setViewport({ width: 1024, height: 768 }) + // console.log('went ot the page') + await page.waitForSelector('[data-testid=rest-category] li div div span') + const restCategories = await page.$$('[data-testid=rest-category] li div div span') + + for (const cat of restCategories) { + await page.evaluate(async (el) => { + return el.click() + }, cat) + + await page.waitForSelector('[data-testid=rest-subcategory]') + } + + const subcategories = await page.evaluate(() => + Array.from(document.querySelectorAll('[data-testid=rest-subcategory] li div div span')).map( + (subCategory) => subCategory.textContent + ) + ) + expect(contentCheck[version].cat.length).toBe(categories.length) + expect(contentCheck[version].subcat.length).toBe(subcategories.length) + + categories.forEach((category) => { + expect(contentCheck[version].cat).toContain(category) + }) + + subcategories.forEach((subcategory) => { + expect(contentCheck[version].subcat).toContain(subcategory) + }) + } + }) +}) + +// Recursively go through the content/rest directory and get all the absolute file names +function getCatAndSubCat(root) { + const files = [] + for (const dirent of fs.readdirSync(root, { withFileTypes: true })) { + const { name } = dirent + const file = path.join(root, name) + if (dirent.isDirectory()) { + if (!(name === 'guides' || name === 'overview')) { + files.push(...getCatAndSubCat(file)) + } + } else if ( + !( + name === 'README.md' || + file.includes('rest/index.md') || + file.includes('rest/quickstart.md') + ) + ) { + files.push(file) + } + } + return files +} + +// Create a ContentCheck object that has all the categories/subcategories and get the title from frontmatter +async function createContentCheckDirectory(contentFiles) { + const contentCheck = Object.keys(allVersions).reduce((acc, val) => { + return { ...acc, [val]: { cat: [], subcat: [] } } + }, {}) + + const renderOpts = { textOnly: true } + + for (const filename of contentFiles) { + const { data } = frontmatter(await fs.promises.readFile(filename, 'utf8')) + const applicableVersions = getApplicableVersions(data.versions, filename) + const splitPath = filename.split('/') + let category = '' + let subCategory = '' + + if (splitPath[splitPath.length - 2] === 'rest') { + category = data.title + } else if (splitPath[splitPath.length - 3] === 'rest') { + filename.includes('index.md') + ? (category = data.shortTitle || data.title) + : (subCategory = data.shortTitle || data.title) + } + for (const version of applicableVersions) { + req.context.currentVersion = version + + if (category !== '') + if (category.includes('{')) { + await shortVersionsMiddleware(req, null, () => {}) + contentCheck[version].cat.push( + await renderContent.liquid.parseAndRender(category, req.context, renderOpts) + ) + } else { + contentCheck[version].cat.push(category) + } + if (subCategory !== '') + if (subCategory.includes('{')) { + await shortVersionsMiddleware(req, null, () => {}) + contentCheck[version].subcat.push( + await renderContent.liquid.parseAndRender(subCategory, req.context, renderOpts) + ) + } else { + contentCheck[version].subcat.push(subCategory) + } + } + } + return contentCheck +} diff --git a/tests/rendering-fixtures/playwright-rendering.spec.ts b/tests/rendering-fixtures/playwright-rendering.spec.ts index 8d922394f145..c8036261c82a 100644 --- a/tests/rendering-fixtures/playwright-rendering.spec.ts +++ b/tests/rendering-fixtures/playwright-rendering.spec.ts @@ -32,8 +32,8 @@ test('use sidebar to go to Hello World page', async ({ page }) => { await page.getByTestId('sidebar').getByRole('link', { name: 'Get started' }).click() await expect(page).toHaveTitle(/Getting started with HubGit/) - await page.getByTestId('product-sidebar-items').getByText('Quickstart').click() - await page.getByTestId('product-sidebar-items').getByRole('link', { name: 'Hello World' }).click() + await page.getByTestId('product-sidebar').getByText('Quickstart').click() + await page.getByTestId('product-sidebar').getByRole('link', { name: 'Hello World' }).click() await expect(page).toHaveURL(/\/en\/get-started\/quickstart\/hello-world/) await expect(page).toHaveTitle(/Hello World - GitHub Docs/) }) diff --git a/tests/rendering-fixtures/sidebar.js b/tests/rendering-fixtures/sidebar.js index 5df2ccf6f45d..aeed0542405d 100644 --- a/tests/rendering-fixtures/sidebar.js +++ b/tests/rendering-fixtures/sidebar.js @@ -11,13 +11,13 @@ describe('sidebar', () => { // Because we know exactly what's in the `externalProducts` // frontmatter property of tests/fixtures/content/index.md // We can predict what to expect to be present. - const links = $('[data-testid=sidebar] a[href="https://github.com"]') + const links = $('[data-testid=sidebar] ul li div div span a[href="https://github.com"]') expect(links.text()).toBe('GitHub itself') }) test('early access is never a link', async () => { const $ = await getDOM('/') - const links = $('[data-testid=sidebar] a[href]') + const links = $('[data-testid=sidebar] ul li div div span a[href]') // The `content/index.md` has to list `early-access` in `children:` // or else those pages won't become part of the site tree. But // they should not actually appear in the side bar. @@ -34,9 +34,13 @@ describe('sidebar', () => { expect($('[data-testid="sidebar-product-xl"]').text()).toBe('Get started') }) - test('leaf-node article marked as data-is-current-page=true', async () => { + test('leaf-node article marked as aria-current=true', async () => { const $ = await getDOM('/get-started/quickstart/hello-world') - expect($('[data-testid=sidebar] [data-is-current-page="true"]').text()).toBe('Hello World') + expect( + $( + '[data-testid=sidebar] [data-testid=sidebar-article-group] li[aria-current="true"]' + ).text() + ).toBe('Hello World') }) test('sidebar should always use the shortTitle', async () => { @@ -50,7 +54,9 @@ describe('sidebar', () => { const $ = await getDOM('/get-started/foo') const link = $('[data-testid=sidebar] a[href*="/get-started/foo/html-short-title"]') expect(link.text()).toBe('GitHub Pages & "GitHub"') - expect(link.html()).toBe('GitHub Pages & "GitHub"') + expect(link.html()).toBe( + '
  • GitHub Pages & "GitHub"
    • ' + ) }) }) }) diff --git a/tests/rendering/sidebar.js b/tests/rendering/sidebar.js index 54ba3d2318e0..8c48b93453dd 100644 --- a/tests/rendering/sidebar.js +++ b/tests/rendering/sidebar.js @@ -1,13 +1,7 @@ -import fs from 'fs' -import path from 'path' import { expect, jest } from '@jest/globals' -import getApplicableVersions from '../../lib/get-applicable-versions.js' -import frontmatter from '../../lib/read-frontmatter.js' import { getDOM } from '../helpers/e2etest.js' import { allVersions } from '../../lib/all-versions.js' -import renderContent from '../../lib/render-content/index.js' -import shortVersionsMiddleware from '../../middleware/contextualizers/short-versions.js' jest.useFakeTimers({ legacyFakeTimers: true }) const req = {} @@ -31,109 +25,16 @@ describe('sidebar', () => { expect($('[data-testid=rest-sidebar-reference]').length).toBe(1) }) - test('Check REST categories and subcategories are rendering', async () => { - // Get the titles from the content/rest directory to match the titles on the page - const contentFiles = getCatAndSubCat('content/rest') - const contentCheck = await createContentCheckDirectory(contentFiles) - - await Promise.all( - Object.keys(allVersions).map(async (version) => { - // Get MapTopic level categories/subcategories for each version on /rest page - const url = `/en/${version}/rest` - const $ = await getDOM(url) - - const categories = [] - $('[data-testid=sidebar] [data-testid=rest-category]').each((i, el) => { - categories[i] = $(el).text() - }) - - const subcategories = [] - $('[data-testid=sidebar] [data-testid=rest-subcategory] a').each((i, el) => { - subcategories[i] = $(el).text() - }) - - expect(contentCheck[version].cat.length).toBe(categories.length) - expect(contentCheck[version].subcat.length).toBe(subcategories.length) - - categories.forEach((category) => { - expect(contentCheck[version].cat).toContain(category) - }) - - subcategories.forEach((subcategory) => { - expect(contentCheck[version].subcat).toContain(subcategory) - }) - }) - ) + test("test a page where there's known sidebar short titles that use Liquid and ampersands", async () => { + const url = + '/en/issues/organizing-your-work-with-project-boards/tracking-work-with-project-boards' + const $ = await getDOM(url) + const linkTexts = [] + $('[data-testid=sidebar] a').each((i, element) => { + linkTexts.push($(element).text()) + }) + // This makes sure that none of the texts in there has their final HTML + // to be HTML entity encoded. + expect(linkTexts.filter((text) => text.includes('&')).length).toBe(0) }) }) - -// Recursively go through the content/rest directory and get all the absolute file names -function getCatAndSubCat(root) { - const files = [] - for (const dirent of fs.readdirSync(root, { withFileTypes: true })) { - const { name } = dirent - const file = path.join(root, name) - if (dirent.isDirectory()) { - if (!(name === 'guides' || name === 'overview')) { - files.push(...getCatAndSubCat(file)) - } - } else if ( - !( - name === 'README.md' || - file.includes('rest/index.md') || - file.includes('rest/quickstart.md') - ) - ) { - files.push(file) - } - } - return files -} - -// Create a ContentCheck object that has all the categories/subcategories and get the title from frontmatter -async function createContentCheckDirectory(contentFiles) { - const contentCheck = Object.keys(allVersions).reduce((acc, val) => { - return { ...acc, [val]: { cat: [], subcat: [] } } - }, {}) - - const renderOpts = { textOnly: true } - - for (const filename of contentFiles) { - const { data } = frontmatter(await fs.promises.readFile(filename, 'utf8')) - const applicableVersions = getApplicableVersions(data.versions, filename) - const splitPath = filename.split('/') - let category = '' - let subCategory = '' - - if (splitPath[splitPath.length - 2] === 'rest') { - category = data.title - } else if (splitPath[splitPath.length - 3] === 'rest') { - filename.includes('index.md') - ? (category = data.shortTitle || data.title) - : (subCategory = data.shortTitle || data.title) - } - for (const version of applicableVersions) { - req.context.currentVersion = version - - if (category !== '') - if (category.includes('{')) { - await shortVersionsMiddleware(req, null, () => {}) - contentCheck[version].cat.push( - await renderContent.liquid.parseAndRender(category, req.context, renderOpts) - ) - } else { - contentCheck[version].cat.push(category) - } - if (subCategory !== '') - if (subCategory.includes('{')) { - await shortVersionsMiddleware(req, null, () => {}) - contentCheck[version].subcat.push( - await renderContent.liquid.parseAndRender(subCategory, req.context, renderOpts) - ) - } else { - contentCheck[version].subcat.push(subCategory) - } - } - } - return contentCheck -}