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

Simplify and unify rustdoc sidebar styles #92692

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jan 9, 2022

Fixes #59860

This switches to just use size, weight, and spacing to distinguish headings in the sidebar. We no longer use boxes, horizontal bars, or centering to distinguish headings. This makes it much easier to understand the hierarchy of headings, and reduces visual noise.

I also refactored how the mobile topbar works. Previously, we tried to shift around elements from the sidebar to make the topbar. Now, the topbar gets its own elements, which can be styled on their own. This makes styling and reasoning about those elements simpler.

Because the heading font sizes are bigger, increase the sidebar width slightly.

As a very minor change, removed version from the "All types" page. It's now only on the crate page.

struct - https://rustdoc.crud.net/jsha/cool-sidebar/std/vec/struct.Vec.html
trait - https://rustdoc.crud.net/jsha/cool-sidebar/std/io/trait.Read.html
crate - https://rustdoc.crud.net/jsha/cool-sidebar/std/index.html
mod - https://rustdoc.crud.net/jsha/cool-sidebar/std/any/index.html
macro - https://rustdoc.crud.net/jsha/cool-sidebar/std/macro.panic.html
fn - https://rustdoc.crud.net/jsha/cool-sidebar/std/io/fn.stdin.html
type alias - https://rustdoc.crud.net/jsha/cool-sidebar/std/io/type.Result.html
keyword - https://rustdoc.crud.net/jsha/cool-sidebar/std/keyword.as.html
primitive - https://rustdoc.crud.net/jsha/cool-sidebar/std/primitive.pointer.html

r? @GuillaumeGomez
cc @camelid
Discussed on Zulip.

Note: This has a lot of smaller commits, but I plan to squash them before merging.

@jsha jsha added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Jan 9, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

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

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

Wouldn't it be better to have the current item name in the sidebar use the color of its type? Because it's currently difficult to make the difference between this one and the other titles. A screenshot:

Screenshot from 2022-01-09 16-16-41

@GuillaumeGomez
Copy link
Member

It'd be nice to change the display of the version because currently it's absolutely the same as the other links in the sidebar:

Screenshot from 2022-01-09 16-19-06

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jan 9, 2022

The burger button is displayed weirdly and is too small I think:
Screenshot from 2022-01-09 16-22-20

Also, the whole header now remains displayed all the time, which is a loss of space. Not sure it's a good idea...

EDIT: The source file viewer sidebar is completely broken currently too.

@GuillaumeGomez
Copy link
Member

Overall I really like the direction it's taking though. The sidebar looks much cleaner. I don't think you should unify it with the source file viewer sidebar though. They have both very different meaning after all.

@GuillaumeGomez
Copy link
Member

When the item name is too long, the sidebar gets a "x scrollbar":

Screenshot from 2022-01-09 16-32-42

Also, If the name is too long, wouldn't it be better to break it instead? For methods I think it's fine, but for types it seems weird. Even more considering how little letters can be displayed otherwise...

@jsha
Copy link
Contributor Author

jsha commented Jan 9, 2022

[on mobile] Also, the whole header now remains displayed all the time, which is a loss of space. Not sure it's a good idea...

I think it's a distinctly good idea. The current design, where only the hamburger menu floats along with the page, feels very broken to me. And it doesn't make the top 45px usable because it overlaps with some of the body text. Also, the topbar provides useful context about where you are, and quick click targets to go to the root of the module (and, later, to the top of the page - which I'd like to add in a separate PR).

@jsha
Copy link
Contributor Author

jsha commented Jan 9, 2022

The current design, where only the hamburger menu floats along with the page, feels very broken to me. And it doesn't make the top 45px usable because it overlaps with some of the body text.

To put this another way: We should either have a topbar that doesn't follow the scroll, or we should have a topbar where the whole width of it follows the scroll. The current situation, where just the hamburger part of the topbar follows the scroll, is unusual UI and doesn't actually get us more usable viewport.

The source file viewer sidebar is completely broken currently too.

What platform, and what page? I checked on both mobile and desktop, and it looks right to me. Did you hard-reload?

It'd be nice to change the display of the version because currently it's absolutely the same as the other links in the sidebar:

In dark mode and ayu, I had forgotten to change the display of links to get the link color. Now the version is distinguished from the links.

Wouldn't it be better to have the current item name in the sidebar use the color of its type? Because it's currently difficult to make the difference between this one and the other titles.

I tried that earlier. It was cool, but in the end I found that it drew too much attention to the sidebar, when the attention should be on the main content. I'll try to distinguish this with better spacing.

The sidebar looks much cleaner. I don't think you should unify it with the source file viewer sidebar though. They have both very different meaning after all.

Yes, my goal was not to touch the source view sidebar in this PR (though I think it could use some touch-ups too).

The burger button is displayed weirdly and is too small I think:

I've confirmed this in Firefox and will fix.

@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Jan 9, 2022

@jsha please rebase instead of merging ;)

@jsha
Copy link
Contributor Author

jsha commented Jan 10, 2022

Alright, I've fixed the source sidebar and the hamburger menu in Firefox Mobile. I also made it so a long item name gets wrapped, and cleaned up some dead code. Updated the demo.

@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Jan 10, 2022

@jsha what do you think about #92692 (comment)?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

What platform, and what page? I checked on both mobile and desktop, and it looks right to me. Did you hard-reload?

All source pages on desktop firefox. It was very likely because of the interactions with the hamburger button. It's working fine now. I should have recorded a video, that would helped to see better the issue...

Also, the topbar provides useful context about where you are, and quick click targets to go to the root of the module (and, later, to the top of the page - which I'd like to add in a separate PR).

I'm not sure what you by this but as always, very curious of what you have in mind. :)

Screenshot from 2022-01-12 10-17-33

Might be a good idea to add some space after the version number to make "all items" easier to see, no?

In dark mode and ayu, I had forgotten to change the display of links to get the link color. Now the version is distinguished from the links.

👍

I tried that earlier. It was cool, but in the end I found that it drew too much attention to the sidebar, when the attention should be on the main content. I'll try to distinguish this with better spacing.

Much better indeed!

Yes, my goal was not to touch the source view sidebar in this PR (though I think it could use some touch-ups too).

I confirm that the sidebar "header" was removed on the source pages. 👍

Once you're done with the changes, please add GUI tests for the following things:

  • colors of the items in the sidebar
  • That for long item name, it doesn't add a scrollbar
  • That the sidebar header is displayed (or not displayed in the case of source pages) and is working as expected on mobile.

And I think that's it? I really can't wait to have all these improvements merged! :D

@bors
Copy link
Contributor

bors commented Jan 13, 2022

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

@jsha
Copy link
Contributor Author

jsha commented Jan 13, 2022

Rebased, squashed, and pushed the demo. I think all concerns have been addressed.

@GuillaumeGomez
Copy link
Member

The font size for the source code pages sidebar has been reduced:

On current nightly:
Screenshot from 2022-01-14 10-44-04

On your PR:
Screenshot from 2022-01-14 10-43-51

Is it expected? In any case, could you add a test to check the font size in the source code sidebar please?

There is margin on the right in mobile when a link is hovered/selected:

Screenshot from 2022-01-14 10-49-09

Is it voluntary?

@jsha
Copy link
Contributor Author

jsha commented Jan 18, 2022

@bors r=GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Jan 18, 2022

📌 Commit 9baf6cb388924538147dabe13180b06cc46c0251 has been approved by 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 Jan 18, 2022
@workingjubilee
Copy link
Member

Somehow, this got set to p=1.
It will likely get rolled up later, and it would be inconvenient if it skipped ahead of rollup=never PRs without being rolled up.

@bors p=0

@bors
Copy link
Contributor

bors commented Jan 18, 2022

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 18, 2022
This switches to just use size, weight, and spacing to distinguish
headings in the sidebar. We no longer use boxes, horizontal bars, or
centering to distinguish headings. This makes it much easier to
understand the hierarchy of headings, and reduces visual noise.

I also refactored how the mobile topbar works. Previously, we tried to
shift around elements from the sidebar to make the topbar. Now, the
topbar gets its own elements, which can be styled on their own. This
makes styling and reasoning about those elements simpler.

Because the heading font sizes are bigger, increase the sidebar width
slightly.

As a very minor change, removed version from the "All types" page. It's
now only on the crate page.
@jsha
Copy link
Contributor Author

jsha commented Jan 18, 2022

@bors r=GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Jan 18, 2022

📌 Commit 6a5f8b1 has been approved by 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 Jan 18, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#90782 (Implement raw-dylib support for windows-gnu)
 - rust-lang#91150 (Let qpath contain NtTy: `<$:ty as $:ty>::…`)
 - rust-lang#92425 (Improve SIMD casts)
 - rust-lang#92692 (Simplify and unify rustdoc sidebar styles)
 - rust-lang#92780 (Directly use ConstValue for single literals in blocks)
 - rust-lang#92924 (Delete pretty printer tracing)
 - rust-lang#93018 (Remove some unused `Ord` derives based on `Span`)
 - rust-lang#93026 (fix typo in `max` description for f32/f64)
 - rust-lang#93035 (Fix stdarch submodule pointing to commit outside tree)

Failed merges:

 - rust-lang#92861 (Rustdoc mobile: put out-of-band info on its own line)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ff476b3 into rust-lang:master Jan 19, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 19, 2022
@jsha jsha deleted the cool-sidebar branch January 19, 2022 03:00
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 20, 2022
…laumeGomez

rustdoc mobile: fix scroll offset when jumping to internal id

Followup to rust-lang#92692. The semantics of `scroll-margin-top` are a little surprising - the attribute needs to be applied to the element that gets scrolled into the viewport, not the scrolling element.

This fixes an issue where clicking on a method (or other item) from the sidebar takes you to a scroll position where the topbar covers up the method name.

I'm interested in ideas for how to test this with browser-ui-test, but I think it doesn't yet have what I need. What I need is an assert that `<element>.getBoundingClientRect().y` is > 45.

Demo: https://rustdoc.crud.net/jsha/fix-scroll-padding-top/std/string/struct.String.html#method.extend_from_within

r? `@GuillaumeGomez`
@nbdd0121
Copy link
Contributor

nbdd0121 commented Jan 24, 2022

I have to say that I really dislike the new style, especially in dark mode. It is much less accessible than previous style.

  • The biggest issue is contrast.

    In the dark theme, the foreground color is #D2991D and the background color of the sidebar is #505050. The computed contrast ratio (using https://webaim.org/resources/contrastchecker/) is 3.19:1 and this violates WCAG AA's requirement of >4.5:1 for normal-sized text.

  • The section division is less clear than the previous style. Color and font size becomes the only way to distinguish.

  • The link color in dark mode really stands out, and having a whole sidebar full of the color made the situation worse.

  • The added padding to the sidebar and warping reduces the information density of the sidebar.

  • Not sure if this is introduced by this PR, but [src] is changed to source. This makes the page feel visually more crowded. Since this is there for all items instead of a link it should be treated as a button (like the run button of code snippets).

Overall for documentation I think accessibility, clarity and easiness to navigate is much more important than style discrepencies.

@GuillaumeGomez
Copy link
Member

There is #93231 open to reduce this issue. Does it improve your situation?

@nbdd0121
Copy link
Contributor

It certainly helps with the contrast issue. But I still think the old UI style is much more usable and pragmatic.

I suggest that an user survey to be performed for such a major UI change.

@camelid
Copy link
Member

camelid commented Jan 25, 2022

Our goal with this change was to make the sidebar easier to use and less cluttered, but I agree that some changes may have made it worse. I do think that decreasing padding and finding a way to increase distinctions between different components of the sidebar would be good.

The [src] -> source change was made in a different PR: #92602

@jsha
Copy link
Contributor Author

jsha commented Jan 25, 2022

FYI, the vertical spacing is precisely the same before and after:

image

The left margin went from to 10px to 24px. However, the max width of the sidebar went from 200px to 250px. As a result, more of the method names are visible than before.

There is indeed a bug with wrapping / ellipsis, which I think was introduced in a different change. I'll send a patch for that shortly.

@nbdd0121
Copy link
Contributor

I use a vertically oriented display for doc viewing, so the added left margin means less horizontal space for the content. Perhaps the left margin should be adaptive to the screen width?

@RDambrosio016
Copy link
Contributor

I personally think that changing the color to be yellow is fine (though i prefer white). However i really dislike the removal of the separator lines, i.e. the border around the struct name and the lines around Methods and similar. Without it it just looks all merged and without clear separation, i just think it looks worse overall.

@pravic
Copy link
Contributor

pravic commented Apr 8, 2022

However i really dislike the removal of the separator lines, i.e. the border around the struct name and the lines around Methods and similar. Without it it just looks all merged and without clear separation, i just think it looks worse overall.

I confirm. The new UI looks as a regression, it's just plain flat. Not that the old UI was perfect, but it was consistent across Rust versions with gradual QOL improvements.

I've created #95816 as a regression report and then was pointed out to this PR.

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.

Simplify and unify rustdoc sidebar styles