TOC in details

[ghost]

Fixes: #36885

Note: small change made via GitHub UI without ant locale testing.

[aduh95]

Also, we need to adjust the stylesheet to the new DOM structure. You should replace

node/doc/api_assets/style.css

Lines 475 to 477 in 1a0c5d5

	#toc h2 {
	margin: 1.5rem 0;
	}

with

#toc > ul {
  margin-top: 1.5rem;
}

[aduh95]

This PR breaks the script that generates the all.html page. I'm suggesting changing

node/tools/doc/allhtml.js

Lines 34 to 37 in 1a0c5d5

	const match = /(<\/ul>\s*)?<\/div>\s*<div id="apicontent">/.exec(data);
	contents += data.slice(0, match.index)
	.replace(/[\s\S]*?<div id="toc">\s*<h2>.*?<\/h2>\s*(<ul>\s*)?/, '');

to

  const match = /(<\/ul>\s*)?<\/\w+>\s*<\w+ id="apicontent">/.exec(data);

  contents += data.slice(0, match.index)
    .replace(/[\s\S]*?id="toc"[^>]*>\s*<\w+>.*?<\/\w+>\s*(<ul>\s*)?/, '')

and

node/tools/doc/allhtml.js

Line 62 in 1a0c5d5

const tocStart = /<div id="toc">\s*<h2>.*?<\/h2>\s*/.exec(all);

to

const tocStart = /<\w+ id="toc"[^>]*>\s*<\w+>.*?<\/\w+>\s*/.exec(all);

[aduh95]

Keep Calm. Lets revert last One first.

I'm confused, why do you want to revert?

[aduh95]

Would it help if I push myself the changes I'm suggesting to your branch to make the CI content?

[aduh95]

@mattiapontonio you actually can add several files in one PR; if you want to do so from GitHub web UI, you need to visit the file on your branch (https://github.com/mattiapontonio/node/blob/patch-1/tools/doc/allhtml.js), click edit, and commit on the same patch-1 branch.
Don't worry about the number of commits in the branch either, our policy is to squash all of them in one commit when merging; and in order to merge a PR the last commit must have a green CI, it doesn't matter if there are red CI commits in the middle.

[aduh95]

I stand by my original comment – repeating "Table of content" is not great, and we should have the open attribute so the TOC is not hidden by default.

You are going to need to update the allhtml.js file as described in my above comment, and also update the style.css before this can land.

[aduh95]

@mattiapontonio are you closing this because you don't want to work on it anymore or did you get block in the process? I hope my comments didn't discourage you, I'm convinced this PR could greatly improve our docs, especially for people browsing them on mobile, and I'm eager to help you land it – if you need or want any help.

[ghost]

@mattiapontonio I hope my comments didn't discourage you

@aduh95
But can you do this fix by touching only one file?

[aduh95]

But can you do this fix by touching only one file?

No, you need to change at least three files. But why is that stopping you? You can modify as many files as you want in one PR by adding more commits to the branch.

[ghost]

I'm from a smartphone...
That's why i opened the issue.
The first CI passed... It was a straight to the point edit, a simple wrapping...

[aduh95]

I'm from a smartphone...

You can do that from the GitHub web CI. I just added a commit to your branch using my smartphone. Try to follow this link and click the edit button: https://github.com/mattiapontonio/node/blob/patch-1/tools/doc/allhtml.js

[ghost]

Yes but i don't want to do that, i want to wrap the TOC in a details...

Lets wait CI for this commit.

[aduh95]

Yes but i don't want to do that, i want to wrap the TOC in a details...

What's the reason you don't want that? Do you think it would look worse or you just don't want to touch the other files because it's not easy to do from a smartphone?

[ghost]

The second one.

Side issue
Markdown should support <select /> and <input type=radio>.

[aduh95]

The second one

OK, so are you OK if I push those changes to your branch myself? I'm afraid that's the only way to get a green CI if you change this part of the HTML.

[ghost]

Yes but please let me see the change (for learning).
If You can merge merge no problem.
Keep the <details/> closed by default for me.

[aduh95]

Keep the <details/> closed by default for me.

You mean you'd prefer without the open attribute? I'm open to have my mind changed, but I'm afraid that might be a surprising change to the docs – there always has been a visible TOC on our docs, hiding it by default would be weird.

/cc @nodejs/documentation what are your thoughts on this?

[ghost]

See MDN for a preview (Jump to section).
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

[ghost]

You mean you'd prefer without the open attribute?

Yes

[ghost]

It's ok for me to leave this open but in the future i could open a new issue about this after testing as a user.

[aduh95]

It's ok for me to leave this open but in the future i could open a new issue about this after testing as a user.

I'd be fine with that too. I was thinking maybe we could add a tiny JS script that automatically removes the open attribute on the <details> element if the user is visiting from a mobile phone on a follow up PR (based on the assumption that scrolling past the TOC is more a concern on smaller screens, but let's see it in practice).

[aduh95]

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4215/

[aduh95]

Landed in 2ba8728

[ghost]

This Is closed not merged 🤔

[ghost]

Landed in 2ba8728

What landed means? ✈️🛬🤔

[ghost]

add a tiny JS script that automatically removes the open attribute on the <details> element if the user is visiting from a mobile phone

I think is better to be consistent between HTMLs

[RaisinTen]

@mattiapontonio your patch has been applied on the HEAD of the master branch instead of performing a merge commit because merges break our tooling.

[ghost]

@mattiapontonio your patch has been applied on the HEAD of the master branch instead of performing a merge commit because merges break our tooling.

Weird. So it's not deployed. And It Will not be?
In never comes in this practice / case

I'm from a smartphone, i can only screeshot what I see here.

I'm expecting something like this

[aduh95]

So it's not deployed. And It Will not be?

It will be part of the next release (well not the next one, because v15.6.0 cut off date has already passed, so the one after that), and then the docs on nodejs.org will be updated with your change. You may use the build artefact from GitHub CI to see the result already: https://github.com/nodejs/node/suites/1829898807/artifacts/35314688

[ghost]

So it's not deployed. And It Will not be?

It will be part of the next release (well not the next one, because v15.6.0 cut off date has already passed, so the one after that), and then the docs on nodejs.org will be updated with your change. You may use the build artefact from GitHub CI to see the result already: https://github.com/nodejs/node/suites/1829898807/artifacts/35314688

Ok 👍

[Trott]

One small issue this creates is that the triangle symbol now means different things (or at least behaves differently) in nearby places in the page. It's different in the version picker drop-down than it is for the table of contents. We might want to think about modifying the version picker slightly.

[ghost]

https://github.com/nodejs/node/suites/1829898807/artifacts/35314688

Should be intresting to see this deployed not in nodejs.org

[ghost]

We might want to think about modifying the version picker slightly.

Where is the HTML of that?
Is a <details> tag?
I meant <details> tag for both.
Disclosure means open and close things, right?

[Trott]

We might want to think about modifying the version picker slightly.

Where is the HTML of that?
Is a <details> tag?
I meant <details> tag for both.
Disclosure means open and close things, right?

No, it's not a details tag.

<a href="#">View another version <span>▼</span></a>

[ghost]

We might want to think about modifying the version picker slightly.

Where is the HTML of that?
Is a <details> tag?
I meant <details> tag for both.
Disclosure means open and close things, right?

No, it's not a details tag.

<a href="#">View another version <span>▼</span></a>

Apparently not even ToC.