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

Fix alignment of method headings for scannability #90155

Merged
merged 1 commit into from
Oct 24, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Oct 22, 2021

We sometimes use indentation to indicate something is a heading: The section that comes after is indented by 24px relative to the heading. However, the relationship between the "Implementations" section heading, the impl headings it contains, and the pub fn subheadings within each impl, is awkward. It goes Implementations, 15px indent, impl, 5px indent, pub fn, 4px indent, docblock.

I line up impl and pub fn with the Implementations heading, give impl a larger font size to indicate it is higher in the hierarchy, and indent the docblock a full 24px relative to their parent, matching the indents we use elsewhere to distinguish section headings. By letting the pub fn stick out to the left of the docblock, I think this makes methods significantly more scannable.

Related to #59829

r? @camelid

Old:

image

New:

image

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2021
@jsha jsha force-pushed the outdent-methods branch 2 times, most recently from c32cadf to 7d1b3fa Compare October 22, 2021 05:39
@GuillaumeGomez
Copy link
Member

In this case, it seems less obvious to me that the method is part of the impl...

@jsha
Copy link
Contributor Author

jsha commented Oct 22, 2021

In this case, it seems less obvious to me that the method is part of the impl...

Yes, this is a downside. We're sacrificing some clarity in the relationship between impl and pub fn in exchange for a lot more clarity in the relationship between pub fn and docblock. Most of the time that you're reading a doc, there are lots of pub fn's in view, but the impl is waaay up top off the screen, so I think the latter is more important.

There's an alternative: We can fully indent for the impl and then again for the pub fn. It would look like this:

image

Do you prefer that? That wasn't my initial approach because it means we have less horizontal space to work with for docblocks, but looking at it now I think it's pretty viable. Some of the feedback on #59829 was that our text is too wide for optimal reading, so we can afford to make it 24px narrower.

@GuillaumeGomez
Copy link
Member

Having less space for the docblock isn't great either... Can we have visual markers maybe? Either an underline for the "impl" or playing with the border like (requires a lot of imagination):

impl bla
|- fn a
|   comment
|- fn b

@jsha
Copy link
Contributor Author

jsha commented Oct 22, 2021

An underline for the impl could work but it would be inconsistent with our current de facto rule that only prose headings get underlines. Here's what it looks like:

I can visualize the tree structure you're suggesting in your diagram, but I think it's not a good solution. Our goal here is to make it easy for people to run their eyes down the "gutter" on the left-hand side of the content, and see where each method begins and ends. Having a lot of stuff in the gutter makes that harder. We want to take stuff away from the gutter rather than adding it.

@GuillaumeGomez
Copy link
Member

Unindenting the fn declaration and keeping the current alignment of the doc comment with the underline on the impl would work for me.

@jsha
Copy link
Contributor Author

jsha commented Oct 22, 2021

Pushed that change and updated the demo (you'll need to hard-refresh to see the updates in the demo).

@camelid
Copy link
Member

camelid commented Oct 22, 2021

I like this change, but the new horizontal rule under the impl header feels unnecessary and looks weird to me:

image

I do feel like we should reduce our use of hrules, and this one in particular just doesn't look right to me. I feel like a better solution would be to add more space between each impl, though I'd be fine with doing that in a future change. But I'd rather not land this with the new hrule.

@jsha
Copy link
Contributor Author

jsha commented Oct 22, 2021

@GuillaumeGomez are you willing to accept this without the hrule? Are there other treatments I should try, like spacing?

@ogoffart
Copy link
Contributor

As an user of the documentation, i often find difficult to see what are the exact type bounds for a function on a generic type. One reason is that it is difficult to find out by scrolling to which impl a function belongs to. Removing the gap will not help, although the extra line might help, but we have to see how usable it is in a complex generic types with many impl and complex bounds.

I can think of a few ideas to improve that:

  • keep two level of indentation like in the second proposal, or maybe increase the font further
  • Always repeat the impl declaration before each functions.
  • keep the impl declaration in a fixed header when scrolling that is always visible.
  • leave the impl out, but repeat the bounds for each functions

I realize each of these ideas have drawback and might not be easy to do, so feel free to ignore me. Just my 2¢.

@jsha
Copy link
Contributor Author

jsha commented Oct 23, 2021

Yep, @ogoffart, I also find it hard to figure out which impl a method is attached to. Of the approaches you propose, I think "keep the impl declaration in a fixed header when scrolling that is always visible" is the most appealing to me - but all of these are quite radical changes. For now I'd like to accomplish the narrower goal of making pub fn's stand out more from their docblocks, without making impls dramatically harder to find. I think that has the most immediate benefit and doesn't stand in the way of later improvements.

@GuillaumeGomez
Copy link
Member

Yes it's fine. It remains an improvement and if needed, we can come back to it later.

@GuillaumeGomez
Copy link
Member

Thanks! r=me and camelid once CI passed.

The makes the heading / documentation distinction clearer.
@camelid camelid added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 23, 2021
@camelid
Copy link
Member

camelid commented Oct 23, 2021

Last comment: Should we keep the slight indent that all implementations used to get? The left side looks a bit tight on space to me. Either way, r=GuillaumeGomez,camelid, since we can always tweak this later.

@camelid camelid added the A-rustdoc-ui Area: Rustdoc UI (generated HTML) label Oct 23, 2021
@jsha
Copy link
Contributor Author

jsha commented Oct 23, 2021

@bors r=GuillaumeGomez,camelid

Should we keep the slight indent that all implementations used to get? The left side looks a bit tight on space to me.

You're right that the left side is a little cramped. But I'd like to follow a consistent pattern: when we use indentation to distinguish a heading from its section, the indent should always be 24px. I think that will make it a lot easier to visually understand the flow of the page. So if we do indent the impl relative to the Implementations prose heading, it should be 24px.

However, we also have the pattern the for prose headings we never use indentations to distinguish the section, so I think bumping in the impl by 24px would be inappropriate.

Perhaps a better option would be to give the gutter overall a bit more space. But also I think this will be less of an issue once we make the toggles less prominent.

@bors
Copy link
Contributor

bors commented Oct 23, 2021

📌 Commit 542ab2d has been approved by GuillaumeGomez,camelid

@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 23, 2021
@camelid
Copy link
Member

camelid commented Oct 23, 2021

However, we also have the pattern the for prose headings we never use indentations to distinguish the section, so I think bumping in the impl by 24px would be inappropriate.

Hmm, I'm not sure why prose headings are relevant here, since "Implementations" is not a user-written heading (I assume that's what you mean by "prose").

Perhaps a better option would be to give the gutter overall a bit more space. But also I think this will be less of an issue once we make the toggles less prominent.

Yeah, I agree that making the toggles less prominent would help.

@jsha
Copy link
Contributor Author

jsha commented Oct 23, 2021

Hmm, I'm not sure why prose headings are relevant here, since "Implementations" is not a user-written heading (I assume that's what you mean by "prose").

I mean as distinguished from code. As I see it we have three kinds of headings:

  1. User-generated prose
  2. Rustdoc-generated prose
  3. Rust identifiers and syntax

1 and 2 tend to follow pretty similar rules - we underline and bold them, and don't indent their following section. That can change if we want it to, of course.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 24, 2021
…mez,camelid

Fix alignment of method headings for scannability

We sometimes use indentation to indicate something is a heading: The section that comes after is indented by 24px relative to the heading. However, the relationship between the "Implementations" section heading, the `impl` headings it contains, and the `pub fn` subheadings within each impl, is awkward. It goes **Implementations**, 15px indent, `impl`, 5px indent, `pub fn`, 4px indent, docblock.

I line up `impl` and `pub fn` with the `Implementations` heading, give `impl` a larger font size to indicate it is higher in the hierarchy, and indent the docblock a full 24px relative to their parent, matching the indents we use elsewhere to distinguish section headings. By letting the `pub fn` stick out to the left of the docblock, I think this makes methods significantly more scannable.

Related to rust-lang#59829

r? `@camelid`

[Old](https://doc.rust-lang.org/nightly/std/string/struct.String.html#implementations):

[![image](https://user-images.githubusercontent.com/220205/138380233-9c63a0f2-0f80-40a3-ab3d-a1ee9fb7c5d8.png)](https://doc.rust-lang.org/nightly/std/string/struct.String.html#implementations)

[New](https://jacob.hoffman-andrews.com/rust/outdent-methods/std/string/struct.String.html#implementations):

[![image](https://user-images.githubusercontent.com/220205/138392479-b45fce3f-bf43-42e0-81ee-c4bb9ac35cda.png)](https://jacob.hoffman-andrews.com/rust/outdent-methods/std/string/struct.String.html#implementations)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 24, 2021
…mez,camelid

Fix alignment of method headings for scannability

We sometimes use indentation to indicate something is a heading: The section that comes after is indented by 24px relative to the heading. However, the relationship between the "Implementations" section heading, the `impl` headings it contains, and the `pub fn` subheadings within each impl, is awkward. It goes **Implementations**, 15px indent, `impl`, 5px indent, `pub fn`, 4px indent, docblock.

I line up `impl` and `pub fn` with the `Implementations` heading, give `impl` a larger font size to indicate it is higher in the hierarchy, and indent the docblock a full 24px relative to their parent, matching the indents we use elsewhere to distinguish section headings. By letting the `pub fn` stick out to the left of the docblock, I think this makes methods significantly more scannable.

Related to rust-lang#59829

r? ``@camelid``

[Old](https://doc.rust-lang.org/nightly/std/string/struct.String.html#implementations):

[![image](https://user-images.githubusercontent.com/220205/138380233-9c63a0f2-0f80-40a3-ab3d-a1ee9fb7c5d8.png)](https://doc.rust-lang.org/nightly/std/string/struct.String.html#implementations)

[New](https://jacob.hoffman-andrews.com/rust/outdent-methods/std/string/struct.String.html#implementations):

[![image](https://user-images.githubusercontent.com/220205/138392479-b45fce3f-bf43-42e0-81ee-c4bb9ac35cda.png)](https://jacob.hoffman-andrews.com/rust/outdent-methods/std/string/struct.String.html#implementations)
This was referenced Oct 24, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 24, 2021
…mez,camelid

Fix alignment of method headings for scannability

We sometimes use indentation to indicate something is a heading: The section that comes after is indented by 24px relative to the heading. However, the relationship between the "Implementations" section heading, the `impl` headings it contains, and the `pub fn` subheadings within each impl, is awkward. It goes **Implementations**, 15px indent, `impl`, 5px indent, `pub fn`, 4px indent, docblock.

I line up `impl` and `pub fn` with the `Implementations` heading, give `impl` a larger font size to indicate it is higher in the hierarchy, and indent the docblock a full 24px relative to their parent, matching the indents we use elsewhere to distinguish section headings. By letting the `pub fn` stick out to the left of the docblock, I think this makes methods significantly more scannable.

Related to rust-lang#59829

r? ```@camelid```

[Old](https://doc.rust-lang.org/nightly/std/string/struct.String.html#implementations):

[![image](https://user-images.githubusercontent.com/220205/138380233-9c63a0f2-0f80-40a3-ab3d-a1ee9fb7c5d8.png)](https://doc.rust-lang.org/nightly/std/string/struct.String.html#implementations)

[New](https://jacob.hoffman-andrews.com/rust/outdent-methods/std/string/struct.String.html#implementations):

[![image](https://user-images.githubusercontent.com/220205/138392479-b45fce3f-bf43-42e0-81ee-c4bb9ac35cda.png)](https://jacob.hoffman-andrews.com/rust/outdent-methods/std/string/struct.String.html#implementations)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 24, 2021
…mez,camelid

Fix alignment of method headings for scannability

We sometimes use indentation to indicate something is a heading: The section that comes after is indented by 24px relative to the heading. However, the relationship between the "Implementations" section heading, the `impl` headings it contains, and the `pub fn` subheadings within each impl, is awkward. It goes **Implementations**, 15px indent, `impl`, 5px indent, `pub fn`, 4px indent, docblock.

I line up `impl` and `pub fn` with the `Implementations` heading, give `impl` a larger font size to indicate it is higher in the hierarchy, and indent the docblock a full 24px relative to their parent, matching the indents we use elsewhere to distinguish section headings. By letting the `pub fn` stick out to the left of the docblock, I think this makes methods significantly more scannable.

Related to rust-lang#59829

r? ````@camelid````

[Old](https://doc.rust-lang.org/nightly/std/string/struct.String.html#implementations):

[![image](https://user-images.githubusercontent.com/220205/138380233-9c63a0f2-0f80-40a3-ab3d-a1ee9fb7c5d8.png)](https://doc.rust-lang.org/nightly/std/string/struct.String.html#implementations)

[New](https://jacob.hoffman-andrews.com/rust/outdent-methods/std/string/struct.String.html#implementations):

[![image](https://user-images.githubusercontent.com/220205/138392479-b45fce3f-bf43-42e0-81ee-c4bb9ac35cda.png)](https://jacob.hoffman-andrews.com/rust/outdent-methods/std/string/struct.String.html#implementations)
This was referenced Oct 24, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2021
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#89558 (Add rustc lint, warning when iterating over hashmaps)
 - rust-lang#90100 (Skip documentation for tier 2 targets on dist-x86_64-apple-darwin)
 - rust-lang#90155 (Fix alignment of method headings for scannability)
 - rust-lang#90162 (Mark `{array, slice}::{from_ref, from_mut}` as const fn)
 - rust-lang#90221 (Fix ICE when forgetting to `Box` a parameter to a `Self::func` call)
 - rust-lang#90234 (Temporarily turn overflow checks off for rustc-rayon-core)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b837605 into rust-lang:master Oct 24, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 24, 2021
@jsha jsha deleted the outdent-methods branch October 27, 2021 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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.

7 participants