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

librustdoc: Use correct heading levels. #89506

Merged
merged 8 commits into from
Oct 6, 2021

Conversation

yaymukund
Copy link
Contributor

Closes #89309

This fixes the <h#> header tags throughout the docs to reflect a semantic hierarchy.

  • I ran a script to manually check that we don't have any files with multiple <h1> tags.
  • Also checked that we never incorrectly nest e.g. a <h2> under an <h3>.
  • I also spot-checked a bunch of pages (trait.Read, enum.Ordering, primitive.isize, trait.Iterator).

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2021
@rust-log-analyzer

This comment has been minimized.

@yaymukund yaymukund marked this pull request as draft October 4, 2021 01:32
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Oct 4, 2021

An invalid file was added: compiler/rustc_error_codes/10. ;)

Also checked that we never incorrectly nest e.g. a <h2> under an <h3>.

Not sure what you meant by that.

So globally, if I understand correctly, you want to prevent <h1> to be added inside doc comments (which is what the issue is explaining as well). Can you add a picture where you show a before/after of a std page with headings please? Please also add a page where all possible headings are display (from <h2> to <h6>).

cc @jsha

- Avoid multiple <h1>s on a page.
- The <h#> tags should follow a semantic hierarchy.
- Cap at h6 (no h7)
@yaymukund
Copy link
Contributor Author

yaymukund commented Oct 4, 2021

Thank you for the review!

I added an invalid file: compiler/rustc_error_codes/10. ;)

Thanks, I've removed it.

Not sure what you meant by that.

I wrote a python script to check that there is never <h3> ... <h2> ... </h2> ... </h3>, which would be invalid html.

So globally, if I understand correctly, you want to prevent <h1> to be added inside doc comments (which is what the issue is explaining as well).

Yes. The hierarchy of <h2>, <h3>, <h4> etc should also make sense. This is described in the original issue (see the 'fn read' example).

Can you add a picture where you show a before/after of a std page with headings please? Please also add a page where all possible headings are display (from <h2> to <h6>).

Yes. Please be aware these screenshots are fairly large (~1.3Mb) so I've linked to them rather than display them inline. I opened them in two tabs, scrolled to the same location, and switched between tabs and they look identical aside from a slight (2px?) shift in the search bar spacing.

master docblock-headings
MaybeUninit Link Link
OpenOptions Link Link
String Link Link

@yaymukund yaymukund marked this pull request as ready for review October 4, 2021 10:16
@GuillaumeGomez
Copy link
Member

I wrote a python script to check that there is never <h3> ... <h2> ... </h2> ... </h3>, which would be invalid html.

That shouldn't be needed, we already have checks for that (you can run them with x.py test html-checker --stage 1).

Yes. Please be aware these screenshots are fairly large (~1.3Mb) so I've linked to them rather than display them inline. I opened them in two tabs, scrolled to the same location, and switched between tabs and they look identical aside from a slight (2px?) shift in the search bar spacing.

Nice, thanks! Btw, you forgot to crop the right side on your last link. ;)

But if I understand correctly, the display shouldn't be changed at all, only the heading level. If so, well done!

@@ -78,6 +80,7 @@ pub struct Markdown<'a>(
/// Default edition to use when parsing doctests (to add a `fn main`).
pub Edition,
pub &'a Option<Playground>,
pub u32,
Copy link
Member

Choose a reason for hiding this comment

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

Please add a doc comment explaining what this field is. Out of context, it's impossible to know what it is without going through the code.

@@ -1339,7 +1362,7 @@ fn render_impl(
} else {
document_item_info(&mut info_buffer, cx, item, Some(parent));
if rendering_params.show_def_docs {
document_full(&mut doc_buffer, item, cx);
document_full(&mut doc_buffer, item, cx, 3);
Copy link
Member

@GuillaumeGomez GuillaumeGomez Oct 4, 2021

Choose a reason for hiding this comment

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

This is a change compared to before. Please add a rustdoc test for it.

@@ -626,7 +626,7 @@ fn item_trait(w: &mut Buffer, cx: &Context<'_>, it: &clean::Item, t: &clean::Tra
let item_type = m.type_();
let id = cx.derive_id(format!("{}.{}", item_type, name));
let mut content = Buffer::empty_from(w);
document(&mut content, cx, m, Some(t));
document_at_level(&mut content, cx, m, Some(t), 3);
Copy link
Member

Choose a reason for hiding this comment

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

Same, this is a change compared to before, please add a rustdoc test for it.

@yaymukund
Copy link
Contributor Author

For some reason, GitHub won't let me 'request a re-review' from notriddle, but all outstanding comments have been addressed. Thanks.

@GuillaumeGomez
Copy link
Member

Tested locally and it looks good to me. Now we just need to wait for @notriddle to take a last look to ensure I didn't miss anything and then it'll be good to go!

Also, no need to re-request reviews, @notriddle had the notifications from your message, they'll come around whenever they have time to. :)

@GuillaumeGomez
Copy link
Member

With this, everyone is onboard.

Thanks @yaymukund!

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 6, 2021

📌 Commit 1f86a8e has been approved by GuillaumeGomez

@bors
Copy link
Contributor

bors commented Oct 6, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2021
@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 6, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 6, 2021
…arth

Rollup of 12 pull requests

Successful merges:

 - rust-lang#87601 (Add functions to add unsigned and signed integers)
 - rust-lang#88523 (Expand documentation for `FpCategory`.)
 - rust-lang#89050 (refactor: VecDeques Drain fields to private)
 - rust-lang#89245 (refactor: make VecDeque's IterMut fields module-private, not just crate-private)
 - rust-lang#89324 (Rename `std::thread::available_conccurrency` to `std::thread::available_parallelism`)
 - rust-lang#89329 (print-type-sizes: skip field printing for primitives)
 - rust-lang#89501 (Note specific regions involved in 'borrowed data escapes' error)
 - rust-lang#89506 (librustdoc: Use correct heading levels.)
 - rust-lang#89528 (Fix suggestion to borrow when casting from pointer to reference)
 - rust-lang#89531 (library std, libc dependency update)
 - rust-lang#89588 (Add a test for generic_const_exprs)
 - rust-lang#89591 (fix: alloc-optimisation is only for rust llvm)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7d6feb4 into rust-lang:master Oct 6, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 6, 2021
@@ -75,6 +75,16 @@ pub(crate) fn summary_opts() -> Options {
| Options::ENABLE_SMART_PUNCTUATION
}

#[derive(Debug, Clone, Copy)]
pub enum HeadingOffset {
H1 = 0,
Copy link
Member

Choose a reason for hiding this comment

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

This looks incorrect to me. H1 should equal 1, no? Otherwise, it seems we'd get <h0>...</h0>.

Copy link
Member

@camelid camelid Oct 6, 2021

Choose a reason for hiding this comment

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

Also, I'd much rather we have a fn level(&self) -> u32 method than relying on enum casting. Casting makes it harder to see where the enum's being converted to a number.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, 0 is correct. The numbers there are added to the level specified in the Markdown file, so “# ghir” gets turned into h1, because it’s (1 + 0).

Copy link
Contributor Author

@yaymukund yaymukund Oct 7, 2021

Choose a reason for hiding this comment

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

It's an offset, so it gets added to the normal heading level. e.g. HeadingOffset::H2 means you add 1 level so # this is an h2 and ## this is an h3. Maybe HeadingStart would be a clearer name?

I agree with you on fn level(&self).

Edit: oops, i did not see you had already responded 😅

Copy link
Member

Choose a reason for hiding this comment

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

I see, that makes sense. Personally, I'd think it'd be clearer to use struct HeadingOffset { offset: u32 } or something like that. Having H1...H6 variants makes it look like it represents <h1> through <h6>.

@yaymukund yaymukund deleted the docblock-headings branch October 8, 2021 02:22
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 25, 2021
…omez

Fix documentation header sizes

And add a rustdoc-gui test confirming various header sizes.

Split off from rust-lang#90156. This fixes a regression in rust-lang#89506 where the heading level of titles within Markdown was too high (h2) for docblocks under structs, unions, and enum impls.

r? `@camelid`

Demo: https://jacob.hoffman-andrews.com/rust/fix-header-sizes/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
Stable: https://doc.rust-lang.org/stable/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
Beta: https://doc.rust-lang.org/beta/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 25, 2021
…omez

Fix documentation header sizes

And add a rustdoc-gui test confirming various header sizes.

Split off from rust-lang#90156. This fixes a regression in rust-lang#89506 where the heading level of titles within Markdown was too high (h2) for docblocks under structs, unions, and enum impls.

r? ``@camelid``

Demo: https://jacob.hoffman-andrews.com/rust/fix-header-sizes/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
Stable: https://doc.rust-lang.org/stable/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
Beta: https://doc.rust-lang.org/beta/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 26, 2021
…omez

Fix documentation header sizes

And add a rustdoc-gui test confirming various header sizes.

Split off from rust-lang#90156. This fixes a regression in rust-lang#89506 where the heading level of titles within Markdown was too high (h2) for docblocks under structs, unions, and enum impls.

r? `@camelid`

Demo: https://jacob.hoffman-andrews.com/rust/fix-header-sizes/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
Stable: https://doc.rust-lang.org/stable/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
Beta: https://doc.rust-lang.org/beta/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2021
Fix documentation header sizes

And add a rustdoc-gui test confirming various header sizes.

Split off from rust-lang#90156. This fixes a regression in rust-lang#89506 where the heading level of titles within Markdown was too high (h2) for docblocks under structs, unions, and enum impls.

r? `@camelid`

Demo: https://jacob.hoffman-andrews.com/rust/fix-header-sizes/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
Stable: https://doc.rust-lang.org/stable/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
Beta: https://doc.rust-lang.org/beta/std/string/struct.String.html#impl-Add%3C%26%27_%20str%3E
notriddle added a commit to notriddle/rust that referenced this pull request Oct 11, 2022
Since 98f05a0 and
b5963f0 removed color classes from sidebar
items, there's no need for the selectors to be so specific any more.

This commit does have to change `h1.fqn a` to just be `h1 a`, so that the
header link color selector is less specific than the typed link at the end.
Since rust-lang#89506 made docblocks start at `h2`, the main page link header should
be the only h1 in the page now.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 11, 2022
…lass, r=GuillaumeGomez

rustdoc: remove unused classes from sidebar links

Since rust-lang@98f05a0 removed separate colors from the currently-selected item, there's no need to have item classes on sidebar links.

Preview: https://notriddle.com/notriddle-rustdoc-demos/sidebar-link-class/std/vec/struct.Vec.html

While cleaning up the CSS to remove unneeded `.content` selectors, this PR changes the `h1.fqn a` CSS selector to just be `h1 a`, so that the header link color selector is less specific than the typed link at the end. Since rust-lang#89506 made docblocks start at `h2`, the main page link header should be the only h1 in the page now.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 12, 2022
…lass, r=GuillaumeGomez

rustdoc: remove unused classes from sidebar links

Since rust-lang@98f05a0 removed separate colors from the currently-selected item, there's no need to have item classes on sidebar links.

Preview: https://notriddle.com/notriddle-rustdoc-demos/sidebar-link-class/std/vec/struct.Vec.html

While cleaning up the CSS to remove unneeded `.content` selectors, this PR changes the `h1.fqn a` CSS selector to just be `h1 a`, so that the header link color selector is less specific than the typed link at the end. Since rust-lang#89506 made docblocks start at `h2`, the main page link header should be the only h1 in the page now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use appropriate heading levels in docblocks
9 participants