Sidebar unification #84834

GuillaumeGomez · 2021-05-02T20:03:43Z

This PR does a few things:

Put crates list at all levels (before, it was only on the "top" items)
Fix bug in module sidebar: the list of items was from the parent module.

The other changes (on bootstrap mostly) were to allow to generate multiple crates in a same folder so that we can ensure that clicking on the crates in the sidebar works as expected.

I added a rustdoc-gui test to ensure everything is where it should be.

r? @jyn514

rust-highfive · 2021-05-02T20:03:45Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

GuillaumeGomez · 2021-05-03T20:10:19Z

src/librustdoc/html/static/main.js

    // delayed sidebar rendering.
    window.initSidebarItems = function(items) {
        var sidebar = document.getElementsByClassName("sidebar-elems")[0];
        var current = window.sidebarCurrent;
+        var isModule = getNakedUrl().endsWith("/index.html");
+
+        function addSidebarCrates(crates) {


I moved this function here so it has access to the sidebar variable from the scope instead of querying it like before.

GuillaumeGomez · 2021-05-03T20:10:52Z

src/librustdoc/html/static/main.js

-        block("keyword", "Keywords");
-        block("traitalias", "Trait Aliases");
+        if (sidebar) {
+            if (!isModule) {


We don't want to render these info on a module (they are already on listed on its page).

GuillaumeGomez · 2021-05-03T20:13:46Z

@jyn514 This is now ready for review. I added a few comments on the changes to help you understand why I did the changes.

bors · 2021-05-07T10:35:48Z

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

jyn514 · 2021-05-09T03:24:32Z

I don't have time to review this.

r? @jsha

GuillaumeGomez · 2021-05-09T11:22:19Z

It's actually not much a change in the front rather than in the back so maybe @camelid might be a better choice here (but any review is very welcome of course!).

r? @camelid

GuillaumeGomez · 2021-05-09T11:26:20Z

And rebased too. :)

GuillaumeGomez · 2021-05-09T15:19:15Z

And of course I pushed the wrong branch once again...

EDIT: fixed

camelid · 2021-05-09T20:35:12Z

Could you include a screenshot of before/after so I can get an idea of what has been changed or fixed?

GuillaumeGomez · 2021-05-28T14:15:41Z

I fixed @Stupremee comment: now I simply check if the body has the mod class to determine if it's a module (crate module also has this class).

So with this, I think it's good?

r? @jsha

bors · 2021-05-30T23:08:20Z

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

jsha · 2021-05-31T05:23:41Z

Linking in some related issues: #85759, #16328, #81031. The theme of these is that some people think the list of crates in the sidebar is much too long. For instance, I just did a quick check on reqwest: it has 71 crates in its sidebar. One of the linked issues mentions servo had 326 in 2018. Adding the crates list to every page would make that problem worse (and would increase page sizes somewhat, for a very rarely used navigational element).

The more I think about it the more I agree with what @Nemo157 said:

the behavior of the "Crates" section appearing anywhere other than at the root of the crate seems like a bug to me, and the fix would be to reduce the places it appears, not adding more.

(BTW sorry if I gave the impression earlier that I fully support the approach here; I'm still thinking aloud about how I think this should work).

We have a hierarchical structure of pages:

crate1
crate2
  struct1
  func1
  moduleA
    struct2
    moduleB
      struct3
    moduleC
      struct4
      struct5

And the question is, at any given point in that structure, what navigation options should we offer in the sidebar? The minimalist view is "breadcrumb" style: from struct4, you have direct links to go to moduleC, moduleA, crate2, or the doc index page. If you want to get from struct4 to struct3, you need to go up to moduleA, then down to moduleB. We have breadcrumbs in place already at the top of the page: in Struct foo::bar::Baz, you can click on foo or bar.

A medium approach would be "siblings" style: from any given node in the hierarchy, you have links to go to all siblings. So struct4 has a link to struct5; moduleB has a link to moduleC; crate1 has a link to crate1.

A maximal approach would be "siblings of all parents:" from any given node, you have links to all siblings, and also the siblings of all your parent nodes.

Right now I think we have a bit of a mix. If you're at struct1, the sidebar will list func1 (a sibling) and moduleA (a sibling), but also crate1 (a parent's sibling).

After writing all that, here's what I think would be consistent:

Sidebar is for siblings; document body is for children. In particular module pages contain their sister modules in the sidebar, and their child modules and types in the document body.
Crates are siblings to each other, so each crate page includes all the crates in its sidebar. Non-crate pages never have crates in their sidebar.

I would also say we have a general problem that the logic of the sidebar is not discoverable. For instance, https://docs.rs/rustls/0.19.1/rustls/struct.DangerousClientConfig.html. The first part of the sidebar is "things that exist on this page." Then it says rustls in a white box and a bunch more items below. It's not very clear what that white box means. It's supposed to mean "we're done with items from this page; below are some items from elsewhere in this crate." It's also not clear how comprehensive that list is: Under "Structs" am I looking at all the structs in this docset? In this crate? (It's in this module, of course). Perhaps one simple fix would be for the white box to say "Other items in rustls."

jsha · 2021-05-31T06:14:54Z

Here's a small prototype of what I think should happen for the crates listing. It uses a regex to determine "on a crate page;" a real version would do better. https://github.com/rust-lang/rust/compare/master...jsha:crates-have-neighbors?expand=1

Demo of generated docs for hyper:

https://hoffman-andrews.com/rust/crates-have-neighbors-hyper/hyper/index.html

Demo of generated docs for library/std:

https://hoffman-andrews.com/rust/crates-have-neighbors/std/

GuillaumeGomez · 2021-05-31T08:17:59Z

I'm a bit sad about the crates being only listed at the top level but if I'm the only one finding it illogical and a bit annoying to go to the crate level to have access to it, then so be it.

Perhaps one simple fix would be for the white box to say "Other items in rustls."

This is a good idea!

It uses a regex to determine "on a crate page;"

I'll instead add a new crate class on the <body> (so the body will have both mod and crate classes) to make things simpler.

* Fix bug in module sidebar: the list of items was from the parent module

…s in rustdoc-gui tool

GuillaumeGomez · 2021-05-31T10:17:10Z

So I did the following:

crate list is now only listed on the crate page.
I added a new crate class on the <body> of the crate page to make it simpler to check if we are on a crate page.
I used @jsha's suggestion for improving the separation between the page's items and siblings (screenshot below)
I added underline when we hover the link in the separation too (to make it more coherent with what we have everywhere else).

src/librustdoc/html/render/context.rs

…the sidebar

jsha · 2021-06-02T06:33:14Z

src/tools/rustdoc-gui/tester.js

+            // This option shows what puppeteer "code" is run
+            // "--debug",
+            // This option disable the headless mode, allowing you to see what's going on.
+            // "--no-headless",
+            // The text isn't rendered by default because of a lot of small differences
+            // between hosts.
+            // "--show-text",


Is this intended to be here but commented out?

Absolutely. Both options are very useful when adding/debugging a test. I have added and removed them a few times already so this time I decided to leave them commented with some doc.

jsha · 2021-06-03T01:21:40Z

@bors r+

Looks good! 🎉

bors · 2021-06-03T01:21:41Z

📌 Commit 9b637fa has been approved by jsha

bors · 2021-06-03T02:31:48Z

⌛ Testing commit 9b637fa with merge 19579c6...

bors · 2021-06-03T05:12:27Z

☀️ Test successful - checks-actions
Approved by: jsha
Pushing 19579c6 to master...

GuillaumeGomez 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 May 2, 2021

rust-highfive assigned jyn514 May 2, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2021

GuillaumeGomez force-pushed the sidebar-unification branch from da3097b to f809c87 Compare May 2, 2021 21:01

rust-lang deleted a comment from rust-log-analyzer May 2, 2021

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the sidebar-unification branch 3 times, most recently from 568474d to 14a5da9 Compare May 3, 2021 20:09

GuillaumeGomez commented May 3, 2021

View reviewed changes

GuillaumeGomez force-pushed the sidebar-unification branch 3 times, most recently from 14a5da9 to 97f4203 Compare May 4, 2021 14:46

rust-highfive assigned jsha and unassigned jyn514 May 9, 2021

rust-highfive assigned camelid and unassigned jsha May 9, 2021

GuillaumeGomez force-pushed the sidebar-unification branch from 97f4203 to c48a19a Compare May 9, 2021 11:26

GuillaumeGomez force-pushed the sidebar-unification branch from c48a19a to 6e72898 Compare May 9, 2021 12:27

GuillaumeGomez force-pushed the sidebar-unification branch from 6e72898 to c48a19a Compare May 9, 2021 15:22

rust-highfive assigned jsha and unassigned camelid May 28, 2021

GuillaumeGomez added 5 commits May 31, 2021 10:30

* Put crates list at all levels

dab01a0

* Fix bug in module sidebar: the list of items was from the parent module

Add sidebar GUI tests

bc888ba

Add some commented options that could be useful for other contributor…

e9b1283

…s in rustdoc-gui tool

Update rustdoc test

5a87482

Remove unneeded file to load sidebar items at crate level

e081cd4

GuillaumeGomez force-pushed the sidebar-unification branch 2 times, most recently from 7149cb5 to 45d9a20 Compare May 31, 2021 10:14

jyn514 reviewed May 31, 2021

View reviewed changes

src/librustdoc/html/render/context.rs Outdated Show resolved Hide resolved

GuillaumeGomez added 2 commits May 31, 2021 15:20

Only list crates on the crate page

a2ba9ef

Improve display of the separation between page items and siblings in …

9b637fa

…the sidebar

GuillaumeGomez force-pushed the sidebar-unification branch from 45d9a20 to 9b637fa Compare May 31, 2021 13:20

jsha reviewed Jun 2, 2021

View reviewed changes

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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 3, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2021

bors merged commit 19579c6 into rust-lang:master Jun 3, 2021

rustbot added this to the 1.54.0 milestone Jun 3, 2021

GuillaumeGomez deleted the sidebar-unification branch June 3, 2021 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sidebar unification #84834

Sidebar unification #84834

GuillaumeGomez commented May 2, 2021 •

edited

Loading

rust-highfive commented May 2, 2021

This comment has been minimized.

GuillaumeGomez May 3, 2021

GuillaumeGomez May 3, 2021

GuillaumeGomez commented May 3, 2021

bors commented May 7, 2021

jyn514 commented May 9, 2021

GuillaumeGomez commented May 9, 2021

GuillaumeGomez commented May 9, 2021

GuillaumeGomez commented May 9, 2021 •

edited

Loading

camelid commented May 9, 2021

GuillaumeGomez commented May 28, 2021

bors commented May 30, 2021

jsha commented May 31, 2021

jsha commented May 31, 2021

GuillaumeGomez commented May 31, 2021 •

edited

Loading

GuillaumeGomez commented May 31, 2021

jsha Jun 2, 2021

GuillaumeGomez Jun 2, 2021 •

edited

Loading

jsha commented Jun 3, 2021

bors commented Jun 3, 2021

bors commented Jun 3, 2021

bors commented Jun 3, 2021

Sidebar unification #84834

Sidebar unification #84834

Conversation

GuillaumeGomez commented May 2, 2021 • edited Loading

rust-highfive commented May 2, 2021

This comment has been minimized.

GuillaumeGomez May 3, 2021

Choose a reason for hiding this comment

GuillaumeGomez May 3, 2021

Choose a reason for hiding this comment

GuillaumeGomez commented May 3, 2021

bors commented May 7, 2021

jyn514 commented May 9, 2021

GuillaumeGomez commented May 9, 2021

GuillaumeGomez commented May 9, 2021

GuillaumeGomez commented May 9, 2021 • edited Loading

camelid commented May 9, 2021

GuillaumeGomez commented May 28, 2021

bors commented May 30, 2021

jsha commented May 31, 2021

jsha commented May 31, 2021

GuillaumeGomez commented May 31, 2021 • edited Loading

GuillaumeGomez commented May 31, 2021

jsha Jun 2, 2021

Choose a reason for hiding this comment

GuillaumeGomez Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

jsha commented Jun 3, 2021

bors commented Jun 3, 2021

bors commented Jun 3, 2021

bors commented Jun 3, 2021

GuillaumeGomez commented May 2, 2021 •

edited

Loading

GuillaumeGomez commented May 9, 2021 •

edited

Loading

GuillaumeGomez commented May 31, 2021 •

edited

Loading

GuillaumeGomez Jun 2, 2021 •

edited

Loading