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

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

Closed
wants to merge 1 commit into from
Closed

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Feb 27, 2021

Motivation

Trying to finally fix #2700.

This PR also introduces the ability to override options for the our own custom Remark plugins (see config file). Perhaps this will come in handy in #4222 to make custom headings ids configurable.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Before After
image image

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Feb 27, 2021
@lex111 lex111 requested a review from slorber as a code owner February 27, 2021 12:14
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 27, 2021
@netlify
Copy link

netlify bot commented Feb 27, 2021

[V1] Deploy preview success

Built with commit 2869379

https://deploy-preview-4310--docusaurus-1.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 27, 2021

Size Change: -25 B (0%)

Total Size: 532 kB

Filename Size Change
website/build/assets/css/styles.********.css 87.1 kB -46 B (0%)
ℹ️ View Unchanged
Filename Size Change
website/build/assets/js/main.********.js 359 kB +21 B (0%)
website/build/blog/2017/12/14/introducing-docusaurus/index.html 60.3 kB 0 B
website/build/docs/introduction/index.html 235 B 0 B
website/build/index.html 25.4 kB 0 B

compressed-size-action

@netlify
Copy link

netlify bot commented Feb 27, 2021

Deploy preview for docusaurus-2 ready!

Built with commit 2869379

https://deploy-preview-4310--docusaurus-2.netlify.app

@github-actions
Copy link

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 90
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

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

@lex111 lex111 added this to the v2.0.0-alpha.71 milestone Mar 4, 2021
Copy link
Collaborator

@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.

Not very satisfied with this solution, as it complicates the remark loader and I see a better way:

  • let the remark plugin computes the TOC with infinite length
  • handle the max depth at render time directly in the theme

This has advantages:

  • simpler to implement and maintain
  • no need to configure anything on the site
  • depth for right TOC can be controlled by md frontmatter
  • depth for inline TOC in MDX can be controlled by prop
  • we can just use maxDepth=2 as default prop to the TOC theme component
  • a theme config can set-up the default TOC depth for the whole site (not just the docs plugin)

@lex111
Copy link
Contributor Author

lex111 commented Mar 4, 2021

I don't quite understand you, we normalize headings (make proper list structure) as it is now, but handle all levels in the loader itself without any options, right?

@slorber
Copy link
Collaborator

slorber commented Mar 4, 2021

I don't quite understand you, we normalize headings (make proper list structure) as it is now, but handle all levels in the loader itself without any options, right?

Not sure what you mean.

Currently, we handle the max depth of the TOC at the mdx loader level, but I think we should not and let the mdx loader compute full TOC without any limit, and apply the max depth limit in the theme instead

@lex111
Copy link
Contributor Author

lex111 commented Mar 4, 2021

In short, I remove code related with configuration. This way the final structure of TOC tree will still be generated in the loader (see the "toc plugin maximum deeply headings" test for example) . And after in TOC component we will only render the appropriate level by filtering initial result, in accordance with the frontmatter settings or defaults. Right?

@slorber
Copy link
Collaborator

slorber commented Mar 4, 2021

exactly :)

@@ -44,28 +40,57 @@ function search(node) {
const onHeading = (child, index, parent) => {
const value = toString(child);

if (parent !== node || !value || child.depth > 3 || child.depth < 2) {
if (
parent !== headingNode ||

Choose a reason for hiding this comment

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

Is it possible (either as part of this PR or a future one) to add a config option to remove this part of the conditional?

I have a use case where I want headings that aren't at the root of the document (in this case, inside a list element) to show in the TOC.

Copy link
Collaborator

@slorber slorber Sep 23, 2021

Choose a reason for hiding this comment

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

Can you explain better your use-case with an example source doc and an example TOC we should compute? (ie provide a test case somehow?)

If we use TOC min/max heading levels for a whole doc, does it work for your use-case? (like min=3, max=4 for example?)

CF #5578

Choose a reason for hiding this comment

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

Sure, see https://raw.githubusercontent.com/australiangreens/agvrules/master/docs/proposed-constitution/01-fundamental-matters.md

The TOC I would like is:

  • Purposes
  • Policies and party strategy
  • Public office
  • Decision-making
  • Openness
  • Affirmative action

Without the suggested change (which I am implementing in the form of a patch), I get no TOC at all, because all the headings are not top-level elements.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not really able to run your doc, can I see a live published version of it too?

What I understand is that you still want min=2 for the toc headings, but you need headings that are part of a list?

  1. Purposes

This is not really an use-case we had in mind afaik. Why can't your readings be top-level and are part of the list?

Is this the intended behavior?

image

Can you explain your motivations for this unusual headings usage?

Copy link

@ineffyble ineffyble Sep 23, 2021

Choose a reason for hiding this comment

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

The published version is at https://australiangreens.github.io/agvrules/proposed-constitution/fundamental-matters/

The headings are part of the list item because:

  • that's how they appear in the source document I'm copying
  • that's semantically correct
  • that structure means that things that parse lists read it correctly

But I think there's also much simpler situations where a heading might not appear at the root of a document. For example, if it's inside a quote block or an admonition. And I can see cases where people would want those to show in the TOC.

My point was these usecases would be very easy to accommodate with an extra config option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @ineffyble

As the TOC PR is already complex enough, I'd like to support your use-case in another one

Can you please open another dedicated issue with all those relevant info?

For example, if it's inside a quote block or an admonition.

I'd find this weird to use a heading this way 😅

And I can see cases where people would want those to show in the TOC.

I'd be very interested to have a list of use-cases that you have in mind where the current logic is not good enough.

Passing a function as TOC config is the most flexible, but it may not be an ideal API to deal with and would be hard to document, so I'm not sure this is what we want. If we supported your use-cases by default it could be good enough?

@erickzhao
Copy link
Contributor

erickzhao commented Sep 16, 2021

👋 Hi there, I'm interested in having this feature in my Docusaurus website and have started working on implementing the change described by @slorber in #4310 (review)

I've re-implemented the search.ts changes in TypeScript so far (with infinite depth), and will need to figure out how to expose the setting in the Classic theme config.

See #5578

@lex111
Copy link
Contributor Author

lex111 commented Sep 17, 2021

Closing in favor of #5578.

@lex111 lex111 closed this Sep 17, 2021
@lex111 lex111 deleted the lex111/iss2700 branch September 24, 2021 21:21
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: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure table-of-contents heading levels (h2, h3, h4...)
5 participants