Skip to content
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

doc: remove TOC summary for pages with no TOC #37043

Merged
merged 1 commit into from
Jan 31, 2021
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 24, 2021

Remove the table of contents summary for pages with no table of
contents. Currently, this affects at least index.html.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jan 24, 2021
@Trott
Copy link
Member Author

Trott commented Jan 24, 2021

Currently, index.html has a Table of Contents detail toggle that does nothing because the page has no table of contents. This fixes that issue.

.use(htmlStringify)
.processSync(toc).toString();
if (toc !== '') {
file.toc = '<details id="toc" open><summary>Table of contents</summary>' +
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR also removes the capitalisation of Contents. That's fine with me, but I wanted to point it out in case it was not on purpose. Other occurrences uses the capitalization:

## Table of Contents

## Table of Contents

Copy link
Member Author

Choose a reason for hiding this comment

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

It was indeed on purpose. Our documentation style guide indicates the use of sentence case for document headings. This isn't exactly a heading (but the other things you point to are). However, the style guide also says to refer to Microsoft's style guide for things it (our style guide) doesn't cover, and Microsoft's style guide recommends sentence case for situations like this.

.processSync(toc).toString() +
'</details>';
} else {
file.toc = '<!-- TOC -->';
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we could rollback to <h2> for the Index:

Suggested change
file.toc = '<!-- TOC -->';
file.toc = '<div id="toc"><h2>Table of contents</h2></div>';

It would let us revert the changes in tools/doc/allhtml.js, but also means the all.html page would not have a retractable TOC anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

all.js is the most useful place for this feature so I'm inclined to do the extra work to keep the <details> tag there.

@Trott
Copy link
Member Author

Trott commented Jan 28, 2021

Welp, the does-nothing-useful-on-the-main-index-page-for-docs <details> tag for Table of Contents is now out on the website as of the last release. (See https://nodejs.org/dist/latest-v15.x/docs/api/ at time of this writing.) I would like to get this landed. Approvals or blocking comments with explanations welcome.

@nodejs/website

@nodejs-github-bot
Copy link
Collaborator

Remove the table of contents summary for pages with no table of
contents. Currently, this affects at least index.html.

PR-URL: nodejs#37043
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@Trott Trott merged commit 683754c into nodejs:master Jan 31, 2021
@Trott
Copy link
Member Author

Trott commented Jan 31, 2021

Landed in 683754c

@Trott Trott deleted the toc branch January 31, 2021 14:05
targos pushed a commit that referenced this pull request Feb 2, 2021
Remove the table of contents summary for pages with no table of
contents. Currently, this affects at least index.html.

PR-URL: #37043
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos mentioned this pull request Feb 2, 2021
targos pushed a commit that referenced this pull request May 1, 2021
Remove the table of contents summary for pages with no table of
contents. Currently, this affects at least index.html.

PR-URL: #37043
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants