-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Documenation Linting and TOC generation #3841
Conversation
|
||
Update TOC | ||
``` | ||
commit 433c58224f5be34480c8e067ca6c5406ba1c1e9c |
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.
code fences are better
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.
Agreed.
1. Use the [GitHub issue search](https://github.com/videojs/video.js/issues) — check if the issue has already been reported. | ||
|
||
1. Check if the issue has already been fixed — try to reproduce it using the latest `master` branch in the repository. | ||
2. Use the [GitHub issue search](https://github.com/videojs/video.js/issues) — check if the issue has already been reported. |
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.
Using 1. for everything makes maintain the doc a lot easier because if you add items or move stuff around you don't need to re-do the entire list.
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.
ok I can change the linting rules to that
* [GitHub personal access token](#github-personal-access-token) | ||
|
||
* [Doing a release](#doing-a-release) | ||
* [Doc credit](#doc-credit) |
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 like this a lot better than the doctoc mess. Does it key off the ## Table of Contents
heading or what?
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.
yeah looks for the first heading called "Table of Contents". We could even rename the heading via an option. If we wanted to. Although I think Table of Contents makes the most sense.
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 looks awesome. Love it.
Any testing considerations or things to think about when looking at the commit. | ||
|
||
Fixes #123. The footer can contain Fixes messages. | ||
```txt |
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 think this one got messed up in the automation. It supposed to look something like
fix: one line commit explanation
In the body of the commit message, we can talk about why we made the change. What the change entails.
Any testing considerations or things to think about when looking at the commit.
Fixes #123. The footer can contain Fixes messages.
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 may have changed it somehow, I put it back to what it was
* [Involving the TSC](#involving-the-tsc) | ||
* [Landing a PR](#landing-a-pr) | ||
* [Landing a PR manually](#landing-a-pr-manually) | ||
|
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.
a lot of whitespace is added to TOCs which I think makes the output look worse.
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.
Looks like the TOC whitespace is a bug. I made a PR to fix it: syntax-tree/mdast-util-toc#3
@@ -77,7 +78,7 @@ The footer should contain things like whether this is a breaking change or what | |||
|
|||
Here's an example: | |||
|
|||
``` | |||
```commit |
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.
huh, didn't realize they had a thing for that.
[issue template]: /.github/ISSUE_TEMPLATE.md | ||
|
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 think this whitespace makes things worse.
[browserstack]: https://browserstack.com | ||
[buildwith]: https://trends.builtwith.com/media/VideoJS | ||
|
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.
👎 to all this whitespace. If these links where visible as is, it might make more sense but it just makes maintaining these a bit worse, I 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.
Doesn't seem like there's an option for remark for this right now. I'll keep my eye on it.
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.
Opened #3882 to track fixing it in the future.
The default component structure of the Video.js player looks something like this: | ||
|
||
``` | ||
```tree |
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.
TIL
## Table of Contents | ||
|
||
* [Standard <video> Element Options](#standard-video-element-options) | ||
* [autoplay](#autoplay) |
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.
Interesting that this stripped out the backticks.
|
||
> Type: `boolean`, Default: `false` |
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.
there needs to be a new line after this because otherwise the Type
and the Note
come out on the same line.
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.
Done.
@@ -25,7 +25,9 @@ | |||
"start": "grunt watchAll", | |||
"test": "grunt test", | |||
"toc": "doctoc", | |||
"docs": "jsdoc -r src/js -d docs/api -c .jsdoc.json" |
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.
docs should probably lint and then generate the api docs. something like
npm run docs:lint && npm run docs:api
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.
Done
@@ -25,7 +25,9 @@ | |||
"start": "grunt watchAll", | |||
"test": "grunt test", | |||
"toc": "doctoc", |
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.
if we're moving to remark for TOCs, we should remove doctoc and this script.
@@ -24,8 +24,10 @@ | |||
"lint": "vjsstandard", | |||
"start": "grunt watchAll", | |||
"test": "grunt test", | |||
"toc": "doctoc", | |||
"docs": "jsdoc -r src/js -d docs/api -c .jsdoc.json" | |||
"docs": "npm run docs:lint && npm run docs:api", |
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.
Should this run docs:fix
?
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 was thinking that this would be something that we could run as part of the build process to lint our markdown files and generate the API docs. In that case, I wouldn't want any other changes to be made to the markdown files. However, the build process could always call these two manually instead.
Description
Use remark to lint, fix formatting of docs, and add a toc if there is a table of contents heading.
Requirements Checklist