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
Docs: Add table of contents to Node.js API docs #9785
Conversation
cb0ca0f
to
718b741
Compare
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.
Generally looks good to me, thanks so much for doing this!
Except for lines I've called out, all of the internal links look great (and they work when I tried them out on your fork).
docs/developer-guide/nodejs-api.md
Outdated
- [defineRules()](#linterdefinerules) | ||
- [getRules()](#lintergetrules) | ||
- [defineParser()](#linterdefineparser) | ||
- [version()](#linterversion) |
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.
This should probably just say [version]
, since this is just a property and not a method.
docs/developer-guide/nodejs-api.md
Outdated
- [CLIEngine](#cliengine) | ||
- [executeOnFiles()](#cliengineexecuteonfiles) | ||
- [resolveFileGlobPatterns()](#cliengineresolvefileglobpatterns) | ||
- [resolveFileGlobPatterns](#cliengineresolveFileGlobPatterns) |
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.
This line appears to be a near-duplicate of the previous line. I think this line can be removed (and the previous line preserved).
This is definitely a great thing to add for now, on a very dense page in our docs. But I would love to see if there is a way this could be automated during site generation (which happens automatically in our release process). I'm okay with merging this in for now, but could we create an issue to track the eventual automation of this table of contents? Thanks! (EDIT: Also, please fix the markdown linting errors. My guess is that the TOC sub-lists should be indented by 4 spaces instead of 2, but there may be others. Please see the Travis and/or AppVeyor status check pages for more info. Thanks!) |
718b741
to
6565efb
Compare
Also standardize headers to ClassName#methodName so that it's easier to keep track of the context.
6565efb
to
d0c5242
Compare
@platinumazure Thanks for the review! The two issues you mentioned as well as Markdown formatting are fixed. I agree, it would be great to automate the TOC in the future. Another useful enhancement would be to validate internal links. |
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.
LGTM, thanks!
Thanks for contributing! |
Also standardize headers to ClassName#methodName so that it's easier to
keep track of the context.
What is the purpose of this pull request? (put an "X" next to item)
[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
ClassName#methodName
convention (instead of justmethodName
)Is there anything you'd like reviewers to focus on?
Some of the internal links have changed. I think I got them all, but could definitely use a second look.