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

Improve display of enum variants #90089

Merged
merged 1 commit into from
Nov 20, 2021
Merged

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Oct 20, 2021

Use h3 and h4 for the variant name and the "Fields" subheading.
Remove the "of T" part of the "Fields" subheading.
Remove border-bottom from "Fields" subheading.
Move docblock below "Fields" listing.

Fixes #90061

Demo:

https://jacob.hoffman-andrews.com/rust/xmlparser-updated/xmlparser/enum.Token.html#variants
https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/io/enum.ErrorKind.html#variants
https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/result/enum.Result.html#variants

r? @camelid

@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 20, 2021
@rust-log-analyzer

This comment has been minimized.

@jsha jsha force-pushed the enum-fields-headings branch 2 times, most recently from 37d9c45 to 53d5ed8 Compare October 20, 2021 07:36
@jsha jsha added the A-rustdoc-ui Area: rustdoc UI (generated HTML) label Oct 20, 2021
@Urgau
Copy link
Contributor

Urgau commented Oct 20, 2021

Just a quick message to say that I think that the layout is (with your changes) even more confusing. I don't know where I should put my eyes. The variant name is so big that it's disturbing, I feel that I should stop at every single of them. The all "Variant" layout feels out of place with the rest of the documentation.

Maybe instead of putting the variant name in h{3,4} you could make it bolder, this would would accentuate the difference but not be as disturbing as your changes.

@jsha
Copy link
Contributor Author

jsha commented Oct 20, 2021

Great feedback, thanks. I'm keeping the h3 / h4 (since that's a structural, not a display issue) but I adjusted the CSS to match your recommendation: now both h3 and h4 for variants are font-size: 16, but h3 is font-weight: 600. This is nicely consistent with how we render method headings.

Updated the demos.

@camelid
Copy link
Member

camelid commented Oct 20, 2021

Can you add a demo for what it looks like when some of the fields have documentation?

@jsha
Copy link
Contributor Author

jsha commented Oct 20, 2021

I updated the xmlparser / Token demo to include documentation on the fields:

https://jacob.hoffman-andrews.com/rust/xmlparser-updated/xmlparser/enum.Token.html#variants

image

@camelid
Copy link
Member

camelid commented Oct 20, 2021

One thing I'd suggest is to add a bit more space between the variant heading and "Fields" and between the fields and the variant's docs. Right now it looks a bit cramped to me.

@jsha
Copy link
Contributor Author

jsha commented Oct 20, 2021

Happy to do so! Would you mind trying out some spacing options in your developer tools and suggesting one that looks good to you? I'm happy to keep iterating on this but I suspect we'll save some round trips this way. :-)

@Urgau
Copy link
Contributor

Urgau commented Oct 20, 2021

Nice, way better.

One thing, it think you can completely remove the "Fields" title (we can clearly tell that they are fields). This would look like this:
Without fields title

This would simplify the layout to only let the important information.

@camelid
Copy link
Member

camelid commented Oct 20, 2021

One thing, it think you can completely remove the "Fields" title (we can clearly tell that they are fields).

I was going to suggest this, but one problem is that for tuple fields, the docs might be confusing. The fields aren't actually named 0, 1, etc., but if you remove that heading, there's no indication that they belong to a tuple variant.

@Urgau
Copy link
Contributor

Urgau commented Oct 20, 2021

The diff from my changes:

- .sub-variant > span {
-    margin-left: 20px;
- }

  .sub-variant {
     margin-left: 24px;
-    margin-bottom: 10px;
+    margin-bottom: 15px;
  }

plus the removal of the h4 "Fields".

@GuillaumeGomez
Copy link
Member

One thing, it think you can completely remove the "Fields" title (we can clearly tell that they are fields).

I was going to suggest this, but one problem is that for tuple fields, the docs might be confusing. The fields aren't actually named 0, 1, etc., but if you remove that heading, there's no indication that they belong to a tuple variant.

Currently they are, so I don't see this as an issue.

@jsha: I find it harder to read the documentation because it's actually not that easy to find where the field starts and where its documentation starts.

@jsha
Copy link
Contributor Author

jsha commented Oct 21, 2021

I've updated the CSS like so:

  • More vertical separation between variant name and the "Fields" heading.
  • More vertical separation between the bottom of the variant's fields and the beginning of the variant's documentation.
  • Leftward indentation is bigger and more consistent with what we do elsewhere: 24px.

I think this should improve visual distinctiveness between the bottom of the fields and the beginning of the doccomment. I don't want to remove the "Fields" heading right now, since I think it is important for explaining the section it comes in front of.

Also I'd like to point out this is intended to be a quick fix for some significant problems with the display of enum field variants (horizontal line inverts the hierarchy; fields show up below doccomment; fields heading is bold, inverting the hierarchy). I always want to try and get the details right, of course, but I also don't want to let a few pixels of whitespace take up too much time here - there are other big display issues I'd like to move on to fixing, also.

@camelid
Copy link
Member

camelid commented Oct 21, 2021

I was going to suggest this, but one problem is that for tuple fields, the docs might be confusing. The fields aren't actually named 0, 1, etc., but if you remove that heading, there's no indication that they belong to a tuple variant.

Currently they are, so I don't see this as an issue.

What do you mean by "currently they are"? The issue I was describing is that in the docs, the fields are displayed as 0: etc., which makes it look like the variant was defined as Variant { 0: i32 }, which is invalid syntax. That's why it seems potentially useful to keep the "Tuple Fields" header, to make it clear that it's actually a tuple variant.

@camelid
Copy link
Member

camelid commented Oct 21, 2021

I've updated the CSS like so: [...]

Ah, this looks much better, thank you! After this change, the docs feel much less cramped to me. I only have two remaining comments about the UI (haven't looked at the impl yet):

  • Putting the fields before the variant's doc-comment seems like it could be confusing. The variant's docs are the summary, while the fields are the details; and in general, one would not put the summary after the details. Can you explain your rationale for this change? I feel that it might be best to leave it as-is for now and just make the heading, spacing, etc. improvements. That way we can make incremental progress without this PR getting stalled on other parts of the change.

  • I think the visual hierarchy would be a bit clearer if the font-size of the variant's name were increased from 1em to 1.1em. What do you think? It looks like this if I edit in devtools:

    image

@jsha
Copy link
Contributor Author

jsha commented Oct 22, 2021

Putting the fields before the variant's doc-comment seems like it could be confusing. The variant's docs are the summary, while the fields are the details; and in general, one would not put the summary after the details. Can you explain your rationale for this change?

In other places, the doccomment is always the last part of a section, and fields / other generated stuff is at the top. Consider the top of the page, for instance - it has struct contents, or enum contents, and then the doccomment underneath. However, I don't feel strongly about this, so I've reverted to the previous behavior of fields on the bottom.

I've also updated the font sizes as you suggest, and pushed the demos.

@GuillaumeGomez
Copy link
Member

I'm realizing now that the [+]/[-] symbols are actually great helper to identify item vs documentation... With just playing on the separation, it's not enough (at least not for me).

Can we have a symbol or symbol or something making it easier to spot the fields?

@jsha
Copy link
Contributor Author

jsha commented Oct 22, 2021

Can you tell me more about what you mean by "identify item vs documentation?" Are you saying that the "Fields" heading and the lines below it look like they are prose documentation written by the doc author? Or that you're scanning the docblock looking for the break between prose and Rusty items, and finding that the break doesn't catch the attention enough?

@GuillaumeGomez
Copy link
Member

Or that you're scanning the docblock looking for the break between prose and Rusty items, and finding that the break doesn't catch the attention enough?

I meant that. It's hard to know if I'm still reading documentation or if it's a field. I have to stop and look a bit around to have this answer, which is not great.

@camelid
Copy link
Member

camelid commented Oct 22, 2021

I'm realizing now that the [+]/[-] symbols are actually great helper to identify item vs documentation... With just playing on the separation, it's not enough (at least not for me).

Can we have a symbol or symbol or something making it easier to spot the fields?

Would moving the fields docs to right below the variant's name like they were earlier in this PR make it easier for you? I agree that the fields look a bit like they're part of the docs themselves. I'd rather put the fields above the variant's docs than add more symbols to the docs though.

@GuillaumeGomez
Copy link
Member

Worth a try!

@jsha
Copy link
Contributor Author

jsha commented Oct 22, 2021

Before I go back to that, I have one more idea to try:

image

This increases the font-weight on Fields to 500, and changes the color to blue. That matches the styling we use for Markdown prose headings elsewhere. Note: The blue indicates a hyperlink, so if we proceed with this I'll change Fields to be a hyperlink that points to its own anchor, the same as other prose headings.

If this is not satisfactory I'm perfectly happy going back to fields-on-top. Let me know which you prefer!

@camelid
Copy link
Member

camelid commented Oct 22, 2021

This increases the font-weight on Fields to 500, and changes the color to blue. That matches the styling we use for Markdown prose headings elsewhere. Note: The blue indicates a hyperlink, so if we proceed with this I'll change Fields to be a hyperlink that points to its own anchor, the same as other prose headings.

I think this adds more complexity and actually makes the docs more confusing. The issue is that "Fields" looks like it's part of the prose, and this makes it look even more like it's part of the prose. Also, I think we should be leaning away from adding more colors and links to the docs.

@jsha
Copy link
Contributor Author

jsha commented Oct 22, 2021

Good point, thanks. Back to fields-on-top. Demos updated - don't forget to hard-refresh.

Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

This is a good improvement, thanks for doing this! I'll review the code soon; for now I have one last UI suggestion.

src/librustdoc/html/static/css/rustdoc.css Outdated Show resolved Hide resolved
@camelid camelid added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2021
@jsha jsha marked this pull request as ready for review October 30, 2021 04:57
@bors
Copy link
Contributor

bors commented Oct 30, 2021

☔ The latest upstream changes (presumably #90422) made this pull request unmergeable. Please resolve the merge conflicts.

Use h3 and h4 for the variant name and the "Fields" subheading.
Remove the "of T" part of the "Fields" subheading.
Remove border-bottom from "Fields" subheading.
Move docblock below "Fields" listing.
@camelid
Copy link
Member

camelid commented Nov 5, 2021

(I'll try to review this soon.)

Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

The Rust changes mostly look fine to me, but I'd like @GuillaumeGomez to review the CSS/HTML tests.

}

document(w, cx, variant, Some(it), HeadingOffset::H4);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, should this be H3?

Copy link
Member

Choose a reason for hiding this comment

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

I think (iirc) that it means the biggest heading will be h4 (please someone correct me if I'm wrong).

Copy link
Member

@camelid camelid Nov 17, 2021

Choose a reason for hiding this comment

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

But I think the heading immediately above (in the HTML, not the Rust source) is h3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is correct. In the demo: https://rustdoc.crud.net/jsha/enum-headings/foo/enum.Token.html#variant.Declaration

"Declaration" is h3, and then "Variant-First" is h4, as it should be.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I guess I'm still confused by HeadingOffset's numbering.

@GuillaumeGomez
Copy link
Member

Looks good to me as well. Once we have confirmation for @camelid's last message, we can approve it. :)

@camelid
Copy link
Member

camelid commented Nov 17, 2021

Once #90089 (comment) is addressed, r=camelid,GuillaumeGomez :)

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2021
@jsha
Copy link
Contributor Author

jsha commented Nov 19, 2021

@bors r=camelid,GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Nov 19, 2021

📌 Commit 69df43b has been approved by camelid,GuillaumeGomez

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 19, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 19, 2021
…d,GuillaumeGomez

Improve display of enum variants

Use h3 and h4 for the variant name and the "Fields" subheading.
Remove the "of T" part of the "Fields" subheading.
Remove border-bottom from "Fields" subheading.
Move docblock below "Fields" listing.

Fixes rust-lang#90061

Demo:

https://jacob.hoffman-andrews.com/rust/xmlparser-updated/xmlparser/enum.Token.html#variants
https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/io/enum.ErrorKind.html#variants
https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/result/enum.Result.html#variants

r? `@camelid`
@camelid
Copy link
Member

camelid commented Nov 19, 2021

Thanks again for making this great UI improvement!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 19, 2021
…d,GuillaumeGomez

Improve display of enum variants

Use h3 and h4 for the variant name and the "Fields" subheading.
Remove the "of T" part of the "Fields" subheading.
Remove border-bottom from "Fields" subheading.
Move docblock below "Fields" listing.

Fixes rust-lang#90061

Demo:

https://jacob.hoffman-andrews.com/rust/xmlparser-updated/xmlparser/enum.Token.html#variants
https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/io/enum.ErrorKind.html#variants
https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/result/enum.Result.html#variants

r? `@camelid`
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 20, 2021
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#88361 (Makes docs for references a little less confusing)
 - rust-lang#90089 (Improve display of enum variants)
 - rust-lang#90956 (Add a regression test for rust-lang#87573)
 - rust-lang#90999 (fix CTFE/Miri simd_insert/extract on array-style repr(simd) types)
 - rust-lang#91026 (rustdoc doctest: detect `fn main` after an unexpected semicolon)
 - rust-lang#91035 (Put back removed empty line)
 - rust-lang#91044 (Turn all 0x1b_u8 into '\x1b' or b'\x1b')
 - rust-lang#91054 (rustdoc: Fix some unescaped HTML tags in docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c0695bb into rust-lang:master Nov 20, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 20, 2021
@jsha jsha deleted the enum-fields-headings branch January 25, 2022 04:32
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.

Fix display of enums with subfields
8 participants