diff --git a/components/context/MainContext.tsx b/components/context/MainContext.tsx index f14b0ac877..51a62d5560 100644 --- a/components/context/MainContext.tsx +++ b/components/context/MainContext.tsx @@ -3,7 +3,6 @@ import pick from 'lodash/pick' import type { BreadcrumbT } from 'components/page-header/Breadcrumbs' import type { FeatureFlags } from 'components/hooks/useFeatureFlags' -import { ExcludesNull } from 'components/lib/ExcludesNull' export type ProductT = { external: boolean @@ -27,14 +26,9 @@ type VersionItem = { } export type ProductTreeNode = { - page: { - hidden?: boolean - documentType: 'article' | 'mapTopic' - title: string - shortTitle: string - } - renderedShortTitle?: string - renderedFullTitle: string + documentType: 'article' | 'mapTopic' + title: string + shortTitle: string href: string childPages: Array } @@ -178,9 +172,10 @@ export const getMainContext = async (req: any, res: any): Promise enterpriseServerVersions: req.context.enterpriseServerVersions, allVersions: req.context.allVersions, currentVersion: req.context.currentVersion, - currentProductTree: req.context.currentProductTree - ? getCurrentProductTree(req.context.currentProductTree) - : null, + // This is a slimmed down version of `req.context.currentProductTree` + // that only has the minimal titles stuff needed for sidebars and + // any page that is hidden is omitted. + currentProductTree: req.context.currentProductTreeTitlesExcludeHidden || null, featureFlags: {}, searchVersions: req.context.searchVersions, nonEnterpriseDefaultVersion: req.context.nonEnterpriseDefaultVersion, @@ -189,26 +184,6 @@ export const getMainContext = async (req: any, res: any): Promise } } -// only pull things we need from the product tree, and make sure there are default values instead of `undefined` -const getCurrentProductTree = (input: any): ProductTreeNode | null => { - if (input.page.hidden) { - return null - } - - return { - href: input.href, - renderedShortTitle: input.renderedShortTitle || '', - renderedFullTitle: input.renderedFullTitle || '', - page: { - hidden: input.page.hidden || false, - documentType: input.page.documentType, - title: input.page.title, - shortTitle: input.page.shortTitle || '', - }, - childPages: (input.childPages || []).map(getCurrentProductTree).filter(ExcludesNull), - } -} - export const MainContext = createContext(null) export const useMainContext = (): MainContextT => { diff --git a/components/context/ProductLandingContext.tsx b/components/context/ProductLandingContext.tsx index 72b045f7a5..9e1108fbf8 100644 --- a/components/context/ProductLandingContext.tsx +++ b/components/context/ProductLandingContext.tsx @@ -110,7 +110,7 @@ export const getProductLandingContextFromRequest = async ( hasGuidesPage, product: { href: productTree.href, - title: productTree.renderedShortTitle || productTree.renderedFullTitle, + title: productTree.page.shortTitle || productTree.page.title, }, whatsNewChangelog: req.context.whatsNewChangelog || [], changelogUrl: req.context.changelogUrl || [], diff --git a/components/landing/ProductArticlesList.tsx b/components/landing/ProductArticlesList.tsx index f1bb878ac9..98856e798a 100644 --- a/components/landing/ProductArticlesList.tsx +++ b/components/landing/ProductArticlesList.tsx @@ -19,7 +19,7 @@ export const ProductArticlesList = () => { return (
{currentProductTree.childPages.map((treeNode, i) => { - if (treeNode.page.documentType === 'article') { + if (treeNode.documentType === 'article') { return null } @@ -36,7 +36,7 @@ const ProductTreeNodeList = ({ treeNode }: { treeNode: ProductTreeNode }) => {

- {treeNode.renderedFullTitle} + {treeNode.title}

@@ -58,8 +58,8 @@ const ProductTreeNodeList = ({ treeNode }: { treeNode: ProductTreeNode }) => { }} > - {childNode.renderedFullTitle} - {childNode.page.documentType === 'mapTopic' ? ( + {childNode.title} + {childNode.documentType === 'mapTopic' ? (  • {childNode.childPages.length} articles diff --git a/components/sidebar/ProductCollapsibleSection.tsx b/components/sidebar/ProductCollapsibleSection.tsx index 3dcd36f068..0f6dad548f 100644 --- a/components/sidebar/ProductCollapsibleSection.tsx +++ b/components/sidebar/ProductCollapsibleSection.tsx @@ -29,7 +29,7 @@ export const ProductCollapsibleSection = (props: SectionProps) => { // The lowest level page link displayed in the tree const renderTerminalPageLink = (page: ProductTreeNode) => { - const title = page.renderedShortTitle || page.renderedFullTitle + const title = page.shortTitle || page.title const isCurrent = routePath === page.href return ( @@ -78,10 +78,10 @@ export const ProductCollapsibleSection = (props: SectionProps) => { { <> {/* */} - {page.childPages[0]?.page.documentType === 'mapTopic' ? ( + {page.childPages[0]?.documentType === 'mapTopic' ? (
    {page.childPages.map((childPage, i) => { - const childTitle = childPage.renderedShortTitle || childPage.renderedFullTitle + const childTitle = childPage.shortTitle || childPage.title const isActive = routePath.includes(childPage.href) const isCurrent = routePath === childPage.href @@ -108,7 +108,7 @@ export const ProductCollapsibleSection = (props: SectionProps) => { ) })}
- ) : page.childPages[0]?.page.documentType === 'article' ? ( + ) : page.childPages[0]?.documentType === 'article' ? (
{page.childPages.map(renderTerminalPageLink)} diff --git a/components/sidebar/RestCollapsibleSection.tsx b/components/sidebar/RestCollapsibleSection.tsx index db9d62ab5d..40dc522b7d 100644 --- a/components/sidebar/RestCollapsibleSection.tsx +++ b/components/sidebar/RestCollapsibleSection.tsx @@ -181,7 +181,7 @@ export const RestCollapsibleSection = (props: SectionProps) => {
) : ( page.childPages.map((childPage, i) => { - const childTitle = childPage.renderedShortTitle || childPage.renderedFullTitle + const childTitle = childPage.shortTitle || childPage.title const isActive = routePath.includes(childPage.href) const isCurrent = routePath === childPage.href diff --git a/components/sidebar/SidebarProduct.tsx b/components/sidebar/SidebarProduct.tsx index 1c2eadeedb..0d82ee76c8 100644 --- a/components/sidebar/SidebarProduct.tsx +++ b/components/sidebar/SidebarProduct.tsx @@ -36,16 +36,16 @@ export const SidebarProduct = () => { routePath.includes(href) ) - const productTitle = currentProductTree.renderedShortTitle || currentProductTree.renderedFullTitle + const productTitle = currentProductTree.shortTitle || currentProductTree.title const productSection = () => (
    • {currentProductTree && currentProductTree.childPages.map((childPage, i) => { - const isStandaloneCategory = childPage.page.documentType === 'article' + const isStandaloneCategory = childPage.documentType === 'article' - const childTitle = childPage.renderedShortTitle || childPage.renderedFullTitle + const childTitle = childPage.shortTitle || childPage.title const isActive = routePath.includes(childPage.href + '/') || routePath === childPage.href const defaultOpen = hasExactCategory ? isActive : false @@ -96,8 +96,8 @@ export const SidebarProduct = () => {
      • {conceptualPages.map((childPage, i) => { - const isStandaloneCategory = childPage.page.documentType === 'article' - const childTitle = childPage.renderedShortTitle || childPage.renderedFullTitle + 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 @@ -145,9 +145,8 @@ export const SidebarProduct = () => {
        • {restPages.map((childPage, i) => { - const isStandaloneCategory = childPage.page.documentType === 'article' - - const childTitle = childPage.renderedShortTitle || childPage.renderedFullTitle + 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 @@ -178,19 +177,15 @@ export const SidebarProduct = () => {
            - {!currentProductTree.page.hidden && ( - <> -
          • - - {productTitle} - -
            • - {currentProduct && currentProduct.id === 'rest' ? restSection() : productSection()} - - )} +
          • + + {productTitle} + +
            • + {currentProduct && currentProduct.id === 'rest' ? restSection() : productSection()}
          ) } diff --git a/lib/page-data.js b/lib/page-data.js index e6b141c873..3af85714ec 100644 --- a/lib/page-data.js +++ b/lib/page-data.js @@ -3,16 +3,12 @@ import path from 'path' import languages from './languages.js' import { allVersions } from './all-versions.js' import createTree, { getBasePath } from './create-tree.js' -import renderContent from './render-content/index.js' import loadSiteData from './site-data.js' import nonEnterpriseDefaultVersion from './non-enterprise-default-version.js' import Page from './page.js' -import shortVersionsMiddleware from '../middleware/contextualizers/short-versions.js' const __dirname = path.dirname(fileURLToPath(import.meta.url)) const versions = Object.keys(allVersions) -const enterpriseServerVersions = versions.filter((v) => v.startsWith('enterprise-server@')) -const renderOpts = { textOnly: true, encodeEntities: true } // These are the exceptions to the rule. // If a URI starts with one of these prefixes, it basically means we don't @@ -103,31 +99,6 @@ export async function versionPages(obj, version, langCode, site) { (pl.pageVersion === 'homepage' && version === nonEnterpriseDefaultVersion) ).href - const req = {} - - req.context = { - allVersions, - enterpriseServerVersions, - currentLanguage: langCode, - currentVersion: version, - site: site[langCode].site, - } - - // The Liquid parseAndRender method is MUCH faster than renderContent or renderProp. - // This only works for titles and short titles, which have no other Markdown that needs - // to be converted to HTML, so we can get away with _only_ parsing Liquid. - await shortVersionsMiddleware(req, null, () => {}) - - obj.renderedFullTitle = obj.page.title.includes('{') - ? await renderContent.liquid.parseAndRender(obj.page.title, req.context, renderOpts) - : obj.page.title - - if (obj.page.shortTitle) { - obj.renderedShortTitle = obj.page.shortTitle.includes('{') - ? await renderContent.liquid.parseAndRender(obj.page.shortTitle, req.context, renderOpts) - : obj.page.shortTitle - } - if (!obj.childPages) return obj const versionedChildPages = await Promise.all( obj.childPages diff --git a/middleware/contextualizers/breadcrumbs.js b/middleware/contextualizers/breadcrumbs.js index ef4195c4c9..3330614ccf 100644 --- a/middleware/contextualizers/breadcrumbs.js +++ b/middleware/contextualizers/breadcrumbs.js @@ -1,6 +1,4 @@ -import liquid from '../../lib/render-content/liquid.js' - -export default async function breadcrumbs(req, res, next) { +export default function breadcrumbs(req, res, next) { if (!req.context.page) return next() const isEarlyAccess = req.context.page.relativePath.startsWith('early-access') if (req.context.page.hidden && !isEarlyAccess) return next() @@ -12,76 +10,81 @@ export default async function breadcrumbs(req, res, next) { return next() } - req.context.breadcrumbs = await getBreadcrumbs(req, isEarlyAccess) + req.context.breadcrumbs = getBreadcrumbs(req, isEarlyAccess) return next() } const earlyAccessExceptions = ['insights', 'enterprise-importer'] -async function getBreadcrumbs(req, isEarlyAccess = false) { - const crumbs = [] - const { currentPath, currentVersion } = req.context - const split = currentPath.split('/') - let cutoff = 2 +function getBreadcrumbs(req, isEarlyAccess) { + let cutoff = 0 + // When in Early access docs consider the "root" be much higher. + // E.g. /en/early-access/github/migrating/understanding/about + // we only want it start at /migrating/understanding/about + // Essentially, we're skipping "/early-access" and its first + // top-level like "/github" if (isEarlyAccess) { - // When in Early access docs consider the "root" be much higher. - // E.g. /en/early-access/github/migrating/understanding/about - // we only want it start at /migrating/understanding/about - // Essentially, we're skipping "/early-access" and its first - // top-level like "/github" - cutoff++ - + const split = req.context.currentPath.split('/') // There are a few exceptions to this rule for the // /{version}/early-access//... URLs because they're a // bit different. // If there are more known exceptions, add them to the array above. - if (!earlyAccessExceptions.some((product) => split.includes(product))) { - cutoff++ - } - - // If the URL is early access AND has a version in it, go even further - // E.g. /en/enterprise-server@3.3/early-access/admin/hosting/mysql - // should start at /hosting/mysql. - if (currentVersion !== 'free-pro-team@latest') { - cutoff++ - } - } - while (split.length > cutoff && split[split.length - 1] !== currentVersion) { - const href = split.join('/') - const page = req.context.pages[href] - if (page) { - crumbs.push({ - href, - title: await getShortTitle(page, req.context), - }) + if (earlyAccessExceptions.some((product) => split.includes(product))) { + cutoff = 1 } else { - console.warn(`No page found with for '${href}'`) + cutoff = 2 } - split.pop() } - crumbs.reverse() + const breadcrumbs = traverseTreeTitles( + req.context.currentPath, + req.context.currentProductTreeTitles + ) + ;[...Array(cutoff)].forEach(() => breadcrumbs.shift()) + + return breadcrumbs +} + +// Return an array as if you'd traverse down a tree. Imagine a tree like +// +// (root /) +// / \ +// (/foo) (/bar) +// / \ +// (/foo/bar) (/foo/buzz) +// +// If the "currentPath" is `/foo/buzz` what you want to return is: +// +// [ +// {href: /, title: TITLE}, +// {href: /foo, title: TITLE} +// {href: /foo/buzz, title: TITLE} +// ] +// +function traverseTreeTitles(currentPath, tree) { + const { href, title, shortTitle } = tree + const crumbs = [ + { + href, + title: shortTitle || title, + }, + ] + const currentPathSplit = Array.isArray(currentPath) ? currentPath : currentPath.split('/') + for (const child of tree.childPages) { + if (isParentOrEqualArray(child.href.split('/'), currentPathSplit)) { + crumbs.push(...traverseTreeTitles(currentPathSplit, child)) + // Only ever going down 1 of the children + break + } + } return crumbs } -async function getShortTitle(page, context) { - // Note! Don't use `page.title` or `page.shortTitle` because if they get - // set during rendering, they become the HTML entities encoded string. - // E.g. "Delete & restore a package" - - if (page.rawShortTitle) { - if (page.rawShortTitle.includes('{')) { - // Can't easily cache this because the `page` is reused for multiple - // permalinks. We could do what the `Page.render()` method does which - // specifically caches based on the `context.currentPath` but at - // this point it's probably not worth it. - return await liquid.parseAndRender(page.rawShortTitle, context) - } - return page.rawShortTitle - } - if (page.rawTitle.includes('{')) { - return await liquid.parseAndRender(page.rawTitle, context) - } - return page.rawTitle +// Return true if an array is part of another array or equal. +// Like `/foo/bar` is part of `/foo/bar/buzz`. +// But also include `/foo/bar/buzz`. +// Don't include `/foo/ba` if the final path is `/foo/baring`. +function isParentOrEqualArray(base, final) { + return base.every((part, i) => part === final[i]) } diff --git a/middleware/contextualizers/current-product-tree.js b/middleware/contextualizers/current-product-tree.js index 3bef4d0d55..4301d19451 100644 --- a/middleware/contextualizers/current-product-tree.js +++ b/middleware/contextualizers/current-product-tree.js @@ -1,9 +1,10 @@ import path from 'path' +import liquid from '../../lib/render-content/liquid.js' import findPageInSiteTree from '../../lib/find-page-in-site-tree.js' import removeFPTFromPath from '../../lib/remove-fpt-from-path.js' // This module adds currentProductTree to the context object for use in layouts. -export default function currentProductTree(req, res, next) { +export default async function currentProductTree(req, res, next) { if (!req.context.page) return next() if (req.context.page.documentType === 'homepage') return next() @@ -20,13 +21,68 @@ export default function currentProductTree(req, res, next) { req.context.currentProduct ) ) - const currentProductTree = findPageInSiteTree( + req.context.currentProductTree = findPageInSiteTree( currentRootTree, req.context.currentEnglishTree, currentProductPath ) - req.context.currentProductTree = currentProductTree + // First make a slim tree of just the 'href', 'title', 'shortTitle' + // 'documentType' and 'childPages' (which is recursive). + // This gets used for map topic and category pages. + req.context.currentProductTreeTitles = await getCurrentProductTreeTitles( + req.context.currentProductTree, + req.context + ) + // Now make an even slimmer version that excludes all hidden pages. + // This is i used for sidebars. + req.context.currentProductTreeTitlesExcludeHidden = excludeHidden( + req.context.currentProductTreeTitles + ) return next() } + +// Return a nested object that contains the bits and pieces we need +// for the tree which is used for sidebars and listing +async function getCurrentProductTreeTitles(input, context) { + const { page } = input + const childPages = await Promise.all( + (input.childPages || []).map((child) => getCurrentProductTreeTitles(child, context)) + ) + + const renderedFullTitle = page.rawTitle.includes('{') + ? await liquid.parseAndRender(page.rawTitle, context) + : page.rawTitle + let renderedShortTitle = '' + if (page.rawShortTitle) { + renderedShortTitle = page.rawShortTitle.includes('{') + ? await liquid.parseAndRender(page.rawShortTitle, context) + : page.rawShortTitle + } + + const node = { + href: input.href, + title: renderedFullTitle, + shortTitle: + renderedShortTitle && (renderedShortTitle || '') !== renderedFullTitle + ? renderedShortTitle + : '', + documentType: page.documentType, + childPages: childPages.filter(Boolean), + } + if (page.hidden) node.hidden = true + return node +} + +function excludeHidden(tree) { + if (tree.hidden) return null + const newTree = { + href: tree.href, + title: tree.title, + shortTitle: tree.shortTitle, + documentType: tree.documentType, + childPages: tree.childPages.map(excludeHidden).filter(Boolean), + } + return newTree +} diff --git a/middleware/contextualizers/generic-toc.js b/middleware/contextualizers/generic-toc.js index fb64cbfd26..569ed661d3 100644 --- a/middleware/contextualizers/generic-toc.js +++ b/middleware/contextualizers/generic-toc.js @@ -1,4 +1,5 @@ import findPageInSiteTree from '../../lib/find-page-in-site-tree.js' +import { liquid } from '../../lib/render-content/index.js' // This module adds either flatTocItems or nestedTocItems to the context object for // product, categorie, and map topic TOCs that don't have other layouts specified. @@ -47,7 +48,7 @@ export default async function genericToc(req, res, next) { const isEarlyAccess = req.context.currentPath.includes('/early-access/') const isArticlesCategory = req.context.currentPath.endsWith('/articles') - req.context.showHiddenTocItems = + const includeHidden = earlyAccessToc || (isCategoryOrMapTopic && isEarlyAccess && !isArticlesCategory) // Conditionally run getTocItems() recursively. @@ -59,49 +60,67 @@ export default async function genericToc(req, res, next) { if (currentTocType === 'flat' && !isOneOffProductToc) { isRecursive = false renderIntros = true - req.context.genericTocFlat = await getTocItems( - treePage.childPages, - req.context, - isRecursive, - renderIntros - ) + req.context.genericTocFlat = [] + req.context.genericTocFlat = await getTocItems(treePage, req.context, { + recurse: isRecursive, + renderIntros, + includeHidden, + }) } // Get an array of child map topics and their child articles and add it to the context object. if (currentTocType === 'nested' || isOneOffProductToc) { isRecursive = !isOneOffProductToc renderIntros = false - req.context.genericTocNested = await getTocItems( - treePage.childPages, - req.context, - isRecursive, - renderIntros - ) + req.context.genericTocNested = await getTocItems(treePage, req.context, { + recurse: isRecursive, + renderIntros, + includeHidden, + }) } return next() } -async function getTocItems(pagesArray, context, isRecursive, renderIntros) { - return ( - await Promise.all( - pagesArray.map(async (child) => { - // only include a hidden page if showHiddenTocItems is true - if (child.page.hidden && !context.showHiddenTocItems) return +// Return a nested object that contains the bits and pieces we need +// for the tree which is used for sidebars and listing +async function getTocItems(node, context, opts) { + // Cleaner than trying to be too terse inside the `.filter()` inline callback. + function filterHidden(child) { + return opts.includeHidden || !child.page.hidden + } - return { - title: child.renderedFullTitle, - fullPath: child.href, - // renderProp is the most expensive part of this function. - intro: renderIntros - ? await child.page.renderProp('intro', context, { unwrap: true }) - : null, - childTocItems: - isRecursive && child.childPages - ? await getTocItems(child.childPages, context, isRecursive, renderIntros) - : null, + return await Promise.all( + node.childPages.filter(filterHidden).map(async (child) => { + const { page } = child + const title = page.rawTitle.includes('{') + ? await liquid.parseAndRender(page.rawTitle, context) + : page.rawTitle + let intro = null + if (opts.renderIntros) { + intro = '' + if (page.rawIntro) { + intro = page.rawIntro.includes('{') + ? await liquid.parseAndRender(page.rawIntro, context) + : page.rawIntro } - }) - ) - ).filter(Boolean) + } + + let childTocItems = null + if (opts.recurse) { + childTocItems = [] + if (child.childPages) { + childTocItems.push(...(await getTocItems(child, context, opts))) + } + } + + const fullPath = child.href + return { + title, + fullPath, + intro, + childTocItems, + } + }) + ) } diff --git a/middleware/index.js b/middleware/index.js index dff852abe9..3a18593264 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -263,9 +263,9 @@ export default function (app) { app.use(instrument(webhooks, './contextualizers/webhooks')) app.use(asyncMiddleware(instrument(whatsNewChangelog, './contextualizers/whats-new-changelog'))) app.use(instrument(layout, './contextualizers/layout')) - app.use(instrument(currentProductTree, './contextualizers/current-product-tree')) + app.use(asyncMiddleware(instrument(currentProductTree, './contextualizers/current-product-tree'))) app.use(asyncMiddleware(instrument(genericToc, './contextualizers/generic-toc'))) - app.use(asyncMiddleware(instrument(breadcrumbs, './contextualizers/breadcrumbs'))) + app.use(instrument(breadcrumbs, './contextualizers/breadcrumbs')) app.use(instrument(features, './contextualizers/features')) app.use(asyncMiddleware(instrument(productExamples, './contextualizers/product-examples'))) app.use(asyncMiddleware(instrument(productGroups, './contextualizers/product-groups'))) diff --git a/tests/content/site-tree.js b/tests/content/site-tree.js index 8d930eb235..b12fb358c9 100644 --- a/tests/content/site-tree.js +++ b/tests/content/site-tree.js @@ -41,9 +41,6 @@ describe('siteTree', () => { expect(pageWithDynamicTitle.page.title).toEqual( 'Installing {% data variables.product.prodname_enterprise %}' ) - - // Confirm a new property contains the rendered title - expect(pageWithDynamicTitle.renderedFullTitle).toEqual('Installing GitHub Enterprise') }) }) diff --git a/tests/rendering/sidebar.js b/tests/rendering/sidebar.js index d8bca619a0..9d4f05454e 100644 --- a/tests/rendering/sidebar.js +++ b/tests/rendering/sidebar.js @@ -173,4 +173,16 @@ describe('sidebar', () => { } } }) + 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) + }) })