Skip to content

add: option to allow headings under certain node type #53

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

Merged
merged 6 commits into from
Feb 20, 2019

Conversation

laysent
Copy link
Contributor

@laysent laysent commented Feb 18, 2019

Hi, I would like to propose this extra option to allow more flexibility of toc generation behavior.

My use case is, when I use remark plugin to wrap each section of article into a section node, headings are no longer on top level, but I would like to still have these headings inside toc.

This change won't break any existing usage.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Nice work! 💪 I added some thoughts inline, but there’s also a bigger, more structural one:

if (parent !== root && parentTypeWhiteList.indexOf(parent.type) < 0) {

Maybe we can do this simpler. Default parents to 'root', that way we can drop the parent !== root part of the condition, which makes usage and code cleaner.

pass settings as a whole, instead of options one by one.
@laysent
Copy link
Contributor Author

laysent commented Feb 19, 2019

Hi, @wooorm, thanks for the feedback, I have made some changes accordingly. Please check details above.
For default value of parents, I think it might be better to use [] and still keeps parent !== root. In this way, users won't accidentally passed in options to overwrite default conditions and cause top-level headings not included. To me, top-level headings should always be included, with or without parents configured.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

@laysent Thanks for the changes, this looks really good! I only have a few tiny suggestions.

To me, top-level headings should always be included, with or without parents configured.

I’m not too sure, I think it’s probably often the case, but maybe someone would only want the ones in section or whatever 🤷‍♂️ It’s a tiny use case, but maybe it’ll show up somewhere the coming years.
And the code for it means less code, so that’s also 👍
But I don’t feel very strongly about it, so let’s see what @BarryThePenguin thinks!

@BarryThePenguin
Copy link
Member

I agree with @wooorm. I'm more inclined to keep the api open to give people the option. Does that mean settings.parents would default to 'root'? And if people wanted they could change it to ['root', 'blockquote'] etc..?

As long as that's mentioned in the documentation I think that's fine 👍

@laysent
Copy link
Contributor Author

laysent commented Feb 19, 2019

Hi, I have made the changes according to the feedback.
I think one thing worth mentioning here: Previously, code checks root !== parent, now it checks !is("root", parent) by default. There will be different result when users passing in a tree where top node is not "root". Probably it's not a case that need to be worried here. Not very sure about that.
If we need to make things work exactly like before, we could set default value of parents to be root element (the first parameter passed in by user) instead of "root".

@wooorm
Copy link
Member

wooorm commented Feb 19, 2019

@laysent You’re right! It would be a breaking change in some cases. I don’t think people ever pass in a non-root as root though, but it could indeed break. So how about settings.parents || root?
To have exactly the same behaviour, with added blockquotes, someone could pass in [root, 'blockquote'] themselves, where root is a reference to the tree passed in. But I’m fine having the docs be simple and mention 'root'

@laysent
Copy link
Contributor Author

laysent commented Feb 19, 2019

@wooorm Sure. Also, still wondering if we should keep typeof settings.parents === 'undefined' ? root : 'root' or simply settings.parents || root. There could be difference when user sets "parents": null. If we keep null as value for parents instead of replacing with "root", it should allow all headings to be included.

@wooorm
Copy link
Member

wooorm commented Feb 19, 2019

I like having null and undefined meaning the same thing, so I think settings.parents || root makes sense. If one were to allow any parent, () => true works and is explicit enough in my opinion!

only headings on second layer of tree will be included
@laysent
Copy link
Contributor Author

laysent commented Feb 20, 2019

Agreed! I have made the changes. :-)

@wooorm
Copy link
Member

wooorm commented Feb 20, 2019

@laysent Wonderful!

@BarryThePenguin What do you think?

Copy link
Member

@BarryThePenguin BarryThePenguin left a comment

Choose a reason for hiding this comment

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

I've got a wip to add tests specifically for the scenarios we discussed above. LGTM 👍 and I'll add another PR

@BarryThePenguin BarryThePenguin merged commit 60c5c1b into syntax-tree:master Feb 20, 2019
BarryThePenguin added a commit that referenced this pull request Feb 20, 2019
Adds test coverage for the examples discussed in #53, specifically when passing nodes that are not `type: "root"` directly to `toc()`
@wooorm wooorm added ⛵️ status/released 🗄 area/interface This affects the public interface 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change labels Aug 12, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

3 participants