feat(v2): allow specifying TOC max depth (themeConfig + frontMatter) by erickzhao · Pull Request #5578 · facebook/docusaurus

erickzhao · 2021-09-16T20:54:38Z

Feature started by @erickzhao, completed by @slorber

Breaking changes

TOC theme components have been refactored for better futureproofness: you might need to upgrade your swizzled components accordingly.
new @theme/TOCItems component
refactored @theme/BlogLayout: toc prop now accepts a <TOC> ReactNode instead of the toc json object.
move useTOCHighlight to theme-common package

Motivation

Fixes #2700

This PR is based on the implementation suggestion from #4310 (review):

let the remark plugin computes the TOC with infinite length

handle the max depth at render time directly in the theme

New frontmatter toc heading level config:

  toc_min_heading_level?: number;
  toc_max_heading_level?: number;

New themeConfig toc heading level (for default levels)

module.exports = {
  themeConfig: {
    tableOfContents: {
      minHeadingLevel: 2,
      minHeadingLevel: 3,
    },
  },
};

Example of toc_max_heading_level:5 on https://docusaurus.io/docs/i18n/crowdin

Have you read the Contributing Guidelines on pull requests?

Yep.

Test Plan

Unit tests

Dogfooding, deploy preview links:

Related PRs

See #4310 for original implementation

erickzhao

A few thoughts on my own implementation :)

erickzhao · 2021-09-16T20:57:17Z

+  depth: headingLevel,
 }: TOCHeadingsProps): JSX.Element | null {
+  const {tableOfContents} = useThemeConfig();
+  const maxDepth = tableOfContents?.maxDepth ?? 3;


Is there a cleaner way of handling default config values? I couldn't find where exactly.

Since you've already added a default value in Joi validation, there's no need to duplicate it here (Joi validation actually mutates the object)
Would it make sense to add a minDepth option as well? Would the user sometimes not want to have h2 headings?

Since you've already added a default value in Joi validation, there's no need to duplicate it here (Joi validation actually mutates the object)

Weird because I get the following error when I don't have the null coalescing part:

TypeError: Cannot destructure property 'maxDepth' of 'tableOfContents' as it is undefined.

git diff of the code change:

diff --git a/packages/docusaurus-theme-classic/src/theme/TOC/index.tsx b/packages/docusaurus-theme-classic/src/theme/TOC/index.tsx index 83c3d2930..4dfe9038c 100644 --- a/packages/docusaurus-theme-classic/src/theme/TOC/index.tsx +++ b/packages/docusaurus-theme-classic/src/theme/TOC/index.tsx @@ -28,7 +28,7 @@ export function TOCHeadings({ depth: headingLevel, }: TOCHeadingsProps): JSX.Element | null { const {tableOfContents} = useThemeConfig(); - const maxDepth = tableOfContents?.maxDepth ?? 3; + const { maxDepth } = tableOfContents; if (!toc.length) { return null; }

Would it make sense to add a minDepth option as well? Would the user sometimes not want to have h2 headings?

Personally I think this feels like an anti-pattern since you generally want to utilize all markdown heading levels. I could see it for some niche cases like having a single h2 on a page and not wanting to nest all of your h3 inside, but feels funny to have it on site-wide in the theme config.

Happy to implement it if others feel differently.

Weird because I get the following error when I don't have the null coalescing part

Oh yeah, you need to provide a default value for the entire tableOfContents config object as well

but feels funny to have it on site-wide in the theme config.

I'm thinking about adding a per-doc option minLevel as well, and just for the sake of uniformity, I think a site-level config is worth it. But seems @slorber doesn't particularly like too many APIs, so let's see🤷‍♂️

Since you've already added a default value in Joi validation, there's no need to duplicate it here (Joi validation actually mutates the object)

Addressed this in 0785423

For consistency I'd rather have min/max everywhere, even if min is less likely to be used and mostly for weird use-cases

netlify · 2021-09-16T21:01:23Z

✔️ [V2]

🔨 Explore the source changes: 3db8779

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61541f09a7aef900077789b1

😎 Browse the preview: https://deploy-preview-5578--docusaurus-2.netlify.app

github-actions · 2021-09-16T21:01:33Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	70
🟢 Accessibility	98
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

Lighthouse ran on https://deploy-preview-5578--docusaurus-2.netlify.app/

Josh-Cena · 2021-09-16T23:23:39Z

+    let parentIndex = prevParentIndex;
+    // We start at the parent of the previous heading
+    // Recurse through its ancestors until the current heading would be a child
+    while (parentIndex >= 0 && currLevel < headings[parentIndex].level) {
+      parentIndex = headings[parentIndex].parentIndex;
    }
+    return parentIndex >= 0 ? headings[parentIndex].parentIndex : parentIndex;
  };

-  visit(node, 'heading', visitor);
+  // Assign the correct `parentIndex` for each heading.
+  headings.forEach((curr, currIndex) => {
+    if (currIndex > 0) {
+      const prevIndex = currIndex - 1;
+      const prev = headings[prevIndex];
+      if (curr.level > prev.level) {
+        curr.parentIndex = prevIndex;
+      } else if (curr.level < prev.level) {
+        curr.parentIndex = getParentIndex(prev.parentIndex, curr.level);
+      } else {
+        curr.parentIndex = prev.parentIndex;
+      }
+    }
+  });


I see a better solution here: record "the nearest h2~h6 node indices" as an array closestNode[] where closestNode[level] is the index of the closest node with level h<level> and closestNode[0] = closestNode[1] = -1.

On each node you find the value in closestNode with a smaller level than curr.level and the largest index possible, and then update closestNode:

const closestNode = Array(7).fill(-1); headings.forEach((curr, currIndex) => { curr.parentIndex = Math.max(...closestNode.slice(0, curr.level)); closestNode[curr.level] = currIndex; });

Of course this would mean a little inline comments, but the code is much shorter and also more straightforward IMO

Josh-Cena · 2021-09-16T23:27:11Z

Invited from Discord :P

LGTM, left some inline comments. Just realized that I gave two contradictory suggestions: using maxDepth to mean the depth of TOC rather than depth of headings and adding a minDepth option🤦‍♂️ I guess the latter is the way to go, and renaming it to maxLevel would make sense

I'd also want a toc_max_level and toc_min_level front matter in Markdown files for more granular control. When you have added this you can dogfood it here: https://deploy-preview-5578--docusaurus-2.netlify.app/docs/sidebar/#sidebar-item-category where the TOC should be deeper than currently

lex111 · 2021-09-17T18:24:42Z

@erickzhao thank you for tackling this issue! Yeah, overriding/setting TOC level on per Markdown doc basis would be awesome. Are you interested in doing this?

erickzhao · 2021-09-18T02:49:08Z

I'll take a look at implementing the per-doc frontmatter config over the weekend. Thanks for the tips! :)

erickzhao · 2021-09-19T23:19:53Z

@Josh-Cena @lex111

I just realized that this current recursion-based approach is flawed if the Markdown doesn't follow W3C heading level guidelines:

Skipping heading ranks can be confusing and should be avoided where possible: Make sure that a <h2> is not followed directly by an <h4>, for example. It is ok to skip ranks when closing subsections, for instance, a <h2> beginning a new section, can follow an <h4> as it closes the previous section.

e.g. If someone skips from h2 to h4 and maxHeadingLevel is h3, the current alg will still recurse twice and pick up the h4 in the TOC.

I see 3 possible solutions to handle incorrectly formatted heading levels:

1️⃣ Refactor the TOCItem type to also contain the heading level and add level-specific logic.
2️⃣ Directly expose maxHeadingDepth instead of maxHeadingLevel.
3️⃣ Assume that the user has correctly-formatted headings (i.e. no level skips) and disclose that the maxHeadingLevel setting won't work well if your heading hierarchy is incorrect.

I think 1️⃣ is the best solution if we do want to also expose the per-document toc_min_heading_level frontmatter option, since we would need to keep track of the levels here anyways.

Josh-Cena · 2021-09-20T00:05:44Z

Yes, 1️⃣ would probably work the best, but I think 3️⃣ will also make sense
Also, I guess the refactor in #5578 (comment) would probably make the code much shorter🧐 Can you check that comment?

Josh-Cena · 2021-09-21T05:58:25Z

+  headings.forEach((curr, currIndex) => {
+    // take the last seen index for each ancestor level. the highest
+    // index will be the direct ancestor of the current heading.
+    const ancestorLevelIndexes = prevIndexForLevel.slice(2, curr.level);


Math.max(...prevIndexForLevel.slice(2, 2)) = Math.max(...[]) = -Infinity, although that doesn't break the algorithm, does it really make sense?

Makes as much sense to me as having indexes for "levels" 0 and 1, I figure. It's nice to be explicit about only starting at index 2 🤔

🤪 my head hurts 🤯 will have to take more time reviewing this.

We need more test cases to cover the edge cases

I tested a few edge cases locally (never committed the changes) and it seemed to work somewhat well.

My personal take (as a drive-by contributor, feel free to ignore) is that while the TOC should never break, documents that have wonky heading formatting won't suffer too much from having a wonky TOC.

Agree, just added tests to ensure the behavior with wonky TOC stays the same over time :)

slorber · 2021-09-28T18:15:20Z

This feature is ready to merge!

Let me know if you see any issue or I'll merge it tomorrow

Josh-Cena · 2021-09-28T23:32:17Z

Typechecking the website coming to use😎

slorber · 2021-09-29T08:15:42Z

Typechecking the website coming to use😎

Yes, not the first error it catches, great idea 👍

lex111 · 2021-09-29T09:26:30Z

+}) {
+  const selectors = [];
+  for (let i = minHeadingLevel; i <= maxHeadingLevel; i += 1) {
+    selectors.push(`.anchor.anchor__h${i}`);


As we discussed it earlier, can we remove this redundant anchor__X class for each of headings levels? Maybe use more specific name like docusaurus_anchor instead of just anchor?

I'm not sure to understand what you mean here.

We want to get only the anchors from min to max, so we need levels in classnames otherwise we'd select more anchors and it could mess-up with the TOC algorithm (an anchor that is not in the TOC should never be considered as "active" because we wouldn't be able to highlight it because it doesn't appear in the TOC)

could mess-up with the TOC algorithm

How can something break if we get all anchor links? If it important, we can actually filter out anchor links based on its heading tag instead class.

It is important. Consider this scenario:

We config to only use h2 in toc (not h3)

We scroll to a h3 heading

Then, the h3 heading is considered the "active" one but it's not in the TOC, so nothing is highlighted (while we should highlight the parent h2)

we can actually filter out anchor links based on its heading tag instead class.

You can do select("h2 h3 h4") or select("*").filter(is("h2 h3 h4")), both would lead to the same result

It's a best practice to create a more precise selector / query in the first place when possible

both would lead to the same result

Yes, but we can get rid of extra classes, why do we need them if can avoid using them?

It doesn't seem like a big deal to me to keep them and would be a CSS breaking change to remove them now

Also nothing guarantees that a user won't use headings in other places of its theme for weird reasons, and we may want to avoid that it conflicts with the TOC algorithm by not using "h2 h3" selector on purpose. Classnames help remove the opportunities for such a conflict

We've already marked this PR as BC, so it's good time to get rid of this extra thing.

Right, but our header-anchors will still have generic class anchor, though could make it even more specific. I suggest removing only heading-specific classes, and use heading tag level instead in TOC algorithm.

Oh I thought you wanted to remove both classes

If you want to refactor to h2.anchor, h3.anchor, I'm fine with the idea (and .anchor__h2 is quite new so it's a smaller breaking change)

…acebook#5578) * feat: add all TOC levels to MDX loader * feat: add theme-level config for heading depth * test: add remark MDX loader test * fix: limit maxDepth validation to H2 - H6 * refactor: set default `maxDepth` using `joi` * refactor: `maxDepth` -> `maxHeadingLevel * refactor: invert underlying TOC depth API * refactor: make TOC algorithm level-aware * feat: add support for per-doc TOC heading levels * feat: support document-level heading levels for blog * fix: correct validation for toc level frontmatter * fix: ensure TOC doesn't generate redundant DOM * perf: simpler TOC heading search alg * docs: document heading level props for `TOCInline` * Update website/docs/guides/markdown-features/markdown-features-inline-toc.mdx Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com> * docs: fix docs (again) * create dedicated test file for heading searching logic: exhaustive tests will be simpler to write * toc search: add real-world test * fix test * add dogfooding tests for toc min/max * add test for min/max toc frontmatter * reverse min/max order * add theme minHeadingLevel + tests * simpler TOC rendering logic * simplify TOC implementation (temp, WIP) * reverse unnatural order for minHeadingLevel/maxHeadingLevel * add TOC dogfooding tests to all content plugins * expose toc min/max heading level frontmatter to all 3 content plugins * refactor blogLayout: accept toc ReactElement directly * move toc utils to theme-common * add tests for filterTOC * create new generic TOCItems component * useless css file copied * fix toc highlighting className conflicts * update doc * fix types Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com> Co-authored-by: slorber <lorber.sebastien@gmail.com>

erickzhao added 4 commits September 16, 2021 10:46

feat: add all TOC levels to MDX loader

b9266ad

feat: add theme-level config for heading depth

9fbef88

test: add remark MDX loader test

fda5103

fix: limit maxDepth validation to H2 - H6

fd1b5f7

erickzhao requested review from lex111 and slorber as code owners September 16, 2021 20:54

facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 16, 2021

erickzhao commented Sep 16, 2021

View reviewed changes

erickzhao mentioned this pull request Sep 16, 2021

feat(v2): allow specifying max depth for TOC #4310

Closed

Josh-Cena reviewed Sep 16, 2021

View reviewed changes

erickzhao mentioned this pull request Sep 16, 2021

Show more levels in the documentation page table of contents electron/website#99

Closed

erickzhao added 2 commits September 16, 2021 20:34

refactor: set default maxDepth using joi

0785423

refactor: maxDepth -> `maxHeadingLevel

7269c0f

Josh-Cena reviewed Sep 18, 2021

View reviewed changes

Comment thread packages/docusaurus-theme-classic/src/theme/TOC/index.tsx Outdated

refactor: invert underlying TOC depth API

213289e

erickzhao added 6 commits September 20, 2021 10:03

refactor: make TOC algorithm level-aware

9c363cc

feat: add support for per-doc TOC heading levels

f4973c2

feat: support document-level heading levels for blog

7a1eeeb

fix: correct validation for toc level frontmatter

f3d61b2

fix: ensure TOC doesn't generate redundant DOM

d2b234a

perf: simpler TOC heading search alg

0a1e91e

Josh-Cena reviewed Sep 21, 2021

View reviewed changes

lex111 reviewed Sep 21, 2021

View reviewed changes

Comment thread website/docs/api/plugins/plugin-content-blog.md

slorber added 14 commits September 24, 2021 17:20

toc search: add real-world test

d3fe29f

fix test

787057a

add dogfooding tests for toc min/max

9ef5679

add test for min/max toc frontmatter

d9fbbd6

reverse min/max order

c581d56

add theme minHeadingLevel + tests

8e8e311

simpler TOC rendering logic

73ad639

simplify TOC implementation (temp, WIP)

90c0943

reverse unnatural order for minHeadingLevel/maxHeadingLevel

40cdce6

add TOC dogfooding tests to all content plugins

f8026f9

expose toc min/max heading level frontmatter to all 3 content plugins

ee8f31c

refactor blogLayout: accept toc ReactElement directly

f222017

move toc utils to theme-common

7e2c562

add tests for filterTOC

9e99c69

slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Sep 28, 2021

create new generic TOCItems component

11b78da

slorber added the pr: new feature This PR adds a new API or behavior. label Sep 28, 2021

slorber added 3 commits September 28, 2021 19:28

useless css file copied

91248eb

fix toc highlighting className conflicts

e01ad12

update doc

7988a43

slorber approved these changes Sep 28, 2021

View reviewed changes

fix types

3db8779

slorber changed the title ~~feat(v2): allow specifying max depth for TOC (theme-level)~~ feat(v2): allow specifying max depth for TOC (themeConfig + frontMatter) Sep 29, 2021

slorber changed the title ~~feat(v2): allow specifying max depth for TOC (themeConfig + frontMatter)~~ feat(v2): allow specifying TOC max depth (themeConfig + frontMatter) Sep 29, 2021

slorber merged commit c86dfbd into facebook:main Sep 29, 2021

lex111 reviewed Sep 29, 2021

View reviewed changes

Uh oh!

Uh oh!

Conversation

erickzhao commented Sep 16, 2021 • edited by slorber Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking changes

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

Uh oh!

erickzhao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

netlify Bot commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Josh-Cena Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Josh-Cena commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lex111 commented Sep 17, 2021

Uh oh!

Uh oh!

erickzhao commented Sep 18, 2021

Uh oh!

erickzhao commented Sep 19, 2021

Uh oh!

Josh-Cena commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slorber commented Sep 28, 2021

Uh oh!

Josh-Cena commented Sep 28, 2021

Uh oh!

slorber commented Sep 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slorber Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erickzhao commented Sep 16, 2021 •

edited by slorber

Loading

netlify Bot commented Sep 16, 2021 •

edited

Loading

github-actions Bot commented Sep 16, 2021 •

edited

Loading

Josh-Cena Sep 16, 2021 •

edited

Loading

Josh-Cena commented Sep 16, 2021 •

edited

Loading

Josh-Cena commented Sep 20, 2021 •

edited

Loading

slorber Sep 29, 2021 •

edited

Loading