Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 TOC max depth (themeConfig + frontMatter) #5578
feat(v2): allow specifying TOC max depth (themeConfig + frontMatter) #5578
Changes from 4 commits
b9266ad
9fbef88
fda5103
fd1b5f7
0785423
7269c0f
213289e
9c363cc
f4973c2
7a1eeeb
f3d61b2
d2b234a
0a1e91e
6ff892d
2abd6a8
a00e19e
7d39b26
d3fe29f
787057a
9ef5679
d9fbbd6
c581d56
8e8e311
73ad639
90c0943
40cdce6
f8026f9
ee8f31c
f222017
7e2c562
9e99c69
11b78da
91248eb
e01ad12
7988a43
3db8779
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a better solution here: record "the nearest
h2
~h6
node indices" as an arrayclosestNode[]
whereclosestNode[level]
is the index of the closest node with levelh<level>
andclosestNode[0] = closestNode[1] = -1
.On each node you find the value in
closestNode
with a smaller level thancurr.level
and the largest index possible, and then updateclosestNode
:Of course this would mean a little inline comments, but the code is much shorter and also more straightforward IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a cleaner way of handling default config values? I couldn't find where exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 haveh2
headings?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird because I get the following error when I don't have the null coalescing part:
git diff
of the code change:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 yourh3
inside, but feels funny to have it on site-wide in the theme config.Happy to implement it if others feel differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, you need to provide a default value for the entire
tableOfContents
config object as wellThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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🤷♂️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this in 0785423
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency I'd rather have min/max everywhere, even if min is less likely to be used and mostly for weird use-cases