-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
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.
Nice work! 💪 I added some thoughts inline, but there’s also a bigger, more structural one:
Line 46 in 37560ad
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.
Hi, @wooorm, thanks for the feedback, I have made some changes accordingly. Please check details above. |
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.
@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!
I agree with @wooorm. I'm more inclined to keep the api open to give people the option. Does that mean As long as that's mentioned in the documentation I think that's fine 👍 |
Hi, I have made the changes according to the feedback. |
@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 |
@wooorm Sure. Also, still wondering if we should keep |
I like having |
only headings on second layer of tree will be included
Agreed! I have made the changes. :-) |
@laysent Wonderful! @BarryThePenguin What do you think? |
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've got a wip to add tests specifically for the scenarios we discussed above. LGTM 👍 and I'll add another PR
Adds test coverage for the examples discussed in #53, specifically when passing nodes that are not `type: "root"` directly to `toc()`
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.