Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(plugin-docs,theme): refactor docs plugin routes and component tree #7966

Merged
merged 15 commits into from
Aug 18, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Aug 17, 2022

Breaking changes

This refactor significantly changes the docs theme component tree. If you swizzled @theme/DocPage, or doc tags pages, you will likely need to upgrade your theme code a bit.

The tree of theme components rendering doc routes changed from:

Root
├── @theme/DocPage
│   └── @theme/DocPage/Layout
│       ├── @theme/DocPage/Layout/Sidebar
│       └── @theme/DocPage/Layout/Main
│           ├── @theme/DocCategoryGeneratedIndexPage
│           └── @theme/DocItem
├── @theme/DocTagsListPage
└── @theme/DocTagDocListPage

to this new structure

Root
└── @theme/DocsRoot
    └── @theme/DocVersionRoot
        ├── @theme/DocRoot
        │   └── @theme/DocRoot/Layout
        │       ├── @theme/DocRoot/Layout/Sidebar
        │       └── @theme/DocRoot/Layout/Main
        │           ├── @theme/DocCategoryGeneratedIndexPage
        │           └── @theme/DocItem
        ├── @theme/DocTagsListPage
        └── @theme/DocTagDocListPage
  • @theme/DocPage has been renamed to @theme/DocRoot
  • @theme/DocRoot (renamed) does not receive a versionMetadata prop anymore. The new @theme/DocVersionRoot receives it (upper in the tree), and makes it available in its subtree with the useDocsVersion() hook.
  • @theme/DocRoot (renamed), @theme/DocTagsListPage and @theme/DocTagDocListPage do not apply <Layout> anymore: it's applied in @theme/DocsLayout (parent of all docs plugin routes)
  • option docPageLayoutComponent has been renamed to docRootComponent

Note there's a clear distinction between:

  • @theme/DocsRoot: top-level parent layout component that wraps every docs plugin routes (all versions, docs, generated index, docs tags pages...) and apply the <Layout> globally
  • @theme/DocRoot: single doc layout, responsible for displaying the docs sidebar alongside the docs content (markdown or category generated index)

Motivation

All the versioned docs pages should have a shared parent layout page. This prevents unmounting/remounting the layout when navigating across docs pages, category-generated index pages, tags pages, and even across versions.

New docs plugin options:

  • docsRootComponent: the component that wraps every route of a single doc plugin instance
  • docVersionRootComponent: the component that wraps every route of a specific doc version

With this refactor, tags-related pages are able to use the useDocsVersion() hook.

Also fixes a bug in the core route sorting algorithm which was not really recursive.

Test Plan

unit tests + preview

Test links

Prod and deploy preview should look exactly the same.

Doc:

Tags list:

Tag page:

@slorber slorber added pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: bug fix This PR fixes a bug in a past release. labels Aug 17, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 17, 2022
@slorber slorber marked this pull request as draft August 17, 2022 18:03
@netlify
Copy link

netlify bot commented Aug 17, 2022

[V2]

Name Link
🔨 Latest commit 5f53573
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62fe56e8c55bc30007836c08
😎 Deploy Preview https://deploy-preview-7966--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Aug 17, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 90 🟢 98 🟢 100 🟢 100 🟠 80 Report
/docs/installation 🟠 79 🟢 100 🟢 100 🟢 100 🟢 90 Report

@github-actions
Copy link

github-actions bot commented Aug 17, 2022

Size Change: +592 B (0%)

Total Size: 814 kB

Filename Size Change
website/build/assets/js/main.********.js 609 kB +592 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 52.5 kB
website/build/assets/css/styles.********.css 111 kB
website/build/index.html 40.8 kB

compressed-size-action

@Josh-Cena
Copy link
Collaborator

Does this supercede #6823?

Copy link
Collaborator Author

@slorber slorber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to review 👍

Comment on lines 261 to 266
export async function createAllRoutes(
param: CreateAllRoutesParam,
): Promise<void> {
const versionRoutes = await buildVersionsRoutes(param);
versionRoutes.forEach((versionRoute) => param.actions.addRoute(versionRoute));
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to move all side-effects here. Ideally our route def API should be improved so that we don't need anymore to use actions.createData() => we should be able to test this route building more easily

@@ -19,7 +18,7 @@ export default function DocPageLayout({children}: Props): JSX.Element {
const sidebar = useDocsSidebar();
const [hiddenSidebarContainer, setHiddenSidebarContainer] = useState(false);
return (
<Layout wrapperClassName={styles.docsWrapper}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout moved upper in the tree: Layout shouldn't unmount/remount when navigating from a doc page to a tag page for example

tag={docVersionSearchTag(version.pluginId, version.version)}
/>
<PageMetadata>
{version.noIndex && <meta name="robots" content="noindex, nofollow" />}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes the limitation described in #7963, because version noIndex is now also applied to tag pages

@@ -63,6 +63,8 @@ export function sortConfig(
});

routeConfigs.forEach((routeConfig) => {
routeConfig.routes?.sort((a, b) => a.path.localeCompare(b.path));
if (routeConfig.routes) {
sortConfig(routeConfig.routes, baseUrl);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated bug in route sorting algorithm: subroutes should be sorted recursively

necessary to make this PR work otherwise /test/docs/ subroute would appear before test/docs/tag/xyz and it fails

Comment on lines +52 to +54
docsRootComponent: '@theme/DocsRoot',
docVersionRootComponent: '@theme/DocVersionRoot',
docRootComponent: '@theme/DocRoot',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New Root theme component we decided with @Josh-Cena .

@theme/DocPage becomes @theme/DocRoot

Hierarchy is: @theme/DocsRoot > @theme/DocVersionRoot > @theme/DocRoot

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slorber
Copy link
Collaborator Author

slorber commented Aug 18, 2022

Thanks, good catch 😅 wonder how I'll fix that

@Josh-Cena
Copy link
Collaborator

Can it be a prop in NotFound to not render Layout?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants