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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Jan 31, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 1 addition & 4 deletions doc/template.html
View file
Open in desktop
Expand Up @@ -55,10 +55,7 @@ <h1>Node.js __VERSION__ Documentation</h1>
<hr>
</header>

<details id="toc" open>
<summary>Table of Contents</summary>
__TOC__
</details>
__TOC__

<div id="apicontent">
__CONTENT__
Expand Down
4 changes: 3 additions & 1 deletion tools/doc/allhtml.js
View file
Open in desktop
Expand Up @@ -59,9 +59,11 @@ let all = toc.replace(/index\.html/g, 'all.html')
all = all.replace(/<title>.*?\| /, '<title>');

// Insert the combined table of contents.
const tocStart = /<\w+ id="toc"[^>]*>\s*<\w+>.*?<\/\w+>\s*/.exec(all);
const tocStart = /<!-- TOC -->/.exec(all);
all = all.slice(0, tocStart.index + tocStart[0].length) +
'<details id="toc" open><summary>Table of contents</summary>\n' +
'<ul>\n' + contents + '</ul>\n' +
'</details>\n' +
all.slice(tocStart.index + tocStart[0].length);

// Replace apicontent with the concatenated set of apicontents from each source.
Expand Down
20 changes: 13 additions & 7 deletions tools/doc/html.js
View file
Open in desktop
Expand Up @@ -382,13 +382,19 @@ function buildToc({ filename, apilinks }) {
node.children.push({ type: 'html', value: anchor });
});

file.toc = unified()
.use(markdown)
.use(gfm)
.use(remark2rehype, { allowDangerousHtml: true })
.use(raw)
.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:

node/doc/guides/cpp-style-guide.md

Line 6 in 7794d36

## Table of Contents

node/doc/guides/releases.md

Line 8 in 4d3e87d

## Table of Contents

node/doc/guides/writing-and-running-benchmarks.md

Line 3 in 860c18b

## 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.

unified()
.use(markdown)
.use(gfm)
.use(remark2rehype, { allowDangerousHtml: true })
.use(raw)
.use(htmlStringify)
.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.

}
};
}

Expand Down