Generate list instead of div items in sidebar #93780

GuillaumeGomez · 2022-02-08T16:06:35Z

Fixes #92986.

Surprisingly, we didn't have much CSS for this...

Demo.

r? @jsha

rust-highfive · 2022-02-08T16:06:37Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd,@jsha

bors · 2022-02-09T10:28:25Z

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

jsha · 2022-02-09T12:37:26Z

Thanks! Can you put up a demo?

GuillaumeGomez · 2022-02-09T13:06:47Z

Sorry, forgot to push it... Here you go: https://rustdoc.crud.net/imperio/links-in-sidebar/std/index.html

jsha

Generally looks great, thanks for working on this!

I mentioned in the review, but I think each portion of the sidebar that has a heading should get its own <div class="block">. That would make the HTML structure more consistent, and also simplify the styling. If we want to get fancy, the <section> element actually seems like a nice semantic replacement for <div class="block">. But I don't feel strongly about whether we use it or not.

By the way, I notice that wherever we use <div class="block">, there's a second class, like block items, block struct etc. Do we ever actually use that second class? Can we get rid of it?

jsha · 2022-02-10T06:35:02Z

src/librustdoc/html/static/css/rustdoc.css

+	white-space: nowrap;
+}
+
+.sidebar-elems .block.items ul:not(last-child),


It's usually best to avoid the :not() selector.

But also, I'm surprised you needed to add this. The existing .block { margin-bottom: 2em } rule should have been enough.

Looking at the HTML, it looks like the whole first part of the sidebar (items on this page) is contained in a single .block. But down below, for items in this module, there's a separate block for each heading. So the structure is like:

<div class="block <foo>"> <h3>...</h3> <ul>...</ul> </div>

We should make the structure up above the same, so there is a separate block for each heading plus the <ul> underneath it. Then you won't need the extra CSS rule here, and the format of the HTML will be more consistent.

It's what I wanted to do at first but I didn't know how to make the different between item's items and the rest. Maybe using <section> will do the trick.

jsha · 2022-02-10T06:37:38Z

src/librustdoc/html/static/css/rustdoc.css

@@ -504,7 +500,11 @@ h2.location a {
 	margin: 0;
 }

-.sidebar-links,
+.sidebar-elems .block.items a {


I think this would be better:

Suggested change

.sidebar-elems .block.items a {

.sidebar-elems li {

The rule doesn't need to be specific to .block.items. For instance, it should apply to the "items in this module" section too. Also: selecting on li instead of a is a nice easy way to say "Headings (h2, h3) can wrap, even when they contain an <a> since we generated them and we know they are of a reasonable length. However, list items are Rust item names; they're out of our control, sometimes quite long, and wrap weirdly. Truncate them rather than wrap."

Currently it's not the case, which is why I only applied it on these items specifically. But if you want, I can do it for all.

GuillaumeGomez · 2022-02-10T19:52:12Z

Now all items in the sidebar are rendered the same way (wrapped in ul itself in a div.block).

I also applied the word-wrap for all li elems in the sidebar making this change:

In the current nightly:

With this PR:

I updated the demo too.

jsha

I love the refactoring to add print_sidebar_block. That's a nice abstraction. Rather than taking a callback, I think it should take impl Iterator<Item = &str>. That would be simpler, and AFAICT all of the inputs can be expressed as iterators.

jsha · 2022-02-16T17:51:54Z

src/librustdoc/html/render/mod.rs

+            "<section class=\"items\">\
+                 <div class=\"block\">\


I was thinking that <section> (no class) would replace <div class=\"block\">. Then the relevant CSS selectors would be .sidebar section.

Oh I see. Even better then!

…div> elements

GuillaumeGomez · 2022-02-16T19:46:01Z

Applied your suggestions! I also updated the demo.

jsha · 2022-02-17T07:02:38Z

@bors r+ rollup

bors · 2022-02-17T07:02:40Z

📌 Commit 0d928b6 has been approved by jsha

…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#93337 (Update tracking issue numbers for inline assembly sub-features) - rust-lang#93758 (Improve comments about type folding/visiting.) - rust-lang#93780 (Generate list instead of div items in sidebar) - rust-lang#93976 (Add MAIN_SEPARATOR_STR) - rust-lang#94011 (Even more let_else adoptions) - rust-lang#94041 (Add a `try_collect()` helper method to `Iterator`) - rust-lang#94043 (Fix ICE when using Box<T, A> with pointer sized A) - rust-lang#94082 (Remove CFG_PLATFORM) - rust-lang#94085 (Clippy: Don't lint `needless_borrow` in method receiver positions) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jsha Feb 8, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Feb 8, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2022

GuillaumeGomez force-pushed the links-in-sidebar branch from ad455d8 to bfb58f4 Compare February 8, 2022 16:19

GuillaumeGomez force-pushed the links-in-sidebar branch from bfb58f4 to 7368c16 Compare February 9, 2022 10:45

jsha requested changes Feb 10, 2022

View reviewed changes

GuillaumeGomez force-pushed the links-in-sidebar branch from 7368c16 to 07d3b6e Compare February 10, 2022 19:48

This comment has been minimized.

# to view

GuillaumeGomez force-pushed the links-in-sidebar branch from 07d3b6e to 5574460 Compare February 10, 2022 20:20

jsha requested changes Feb 16, 2022

View reviewed changes

GuillaumeGomez and others added 2 commits February 16, 2022 20:32

Unify sidebar a bit more by generating a list using <ul> instead of <…

5136b54

…div> elements

Update rustdoc tests to adapt to changes to the sidebar

0d928b6

GuillaumeGomez force-pushed the links-in-sidebar branch from 5574460 to 0d928b6 Compare February 16, 2022 19:43

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 Feb 17, 2022

matthiaskrgr mentioned this pull request Feb 17, 2022

Rollup of 9 pull requests #94103

Merged

bors merged commit 6a2b612 into rust-lang:master Feb 18, 2022

rustbot added this to the 1.60.0 milestone Feb 18, 2022

GuillaumeGomez deleted the links-in-sidebar branch February 18, 2022 09:30

Generate list instead of div items in sidebar #93780

Generate list instead of div items in sidebar #93780

Uh oh!

Conversation

GuillaumeGomez commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Feb 8, 2022

Uh oh!

bors commented Feb 9, 2022

Uh oh!

jsha commented Feb 9, 2022 via email

Uh oh!

GuillaumeGomez commented Feb 9, 2022

Uh oh!

jsha left a comment

Choose a reason for hiding this comment

Uh oh!

jsha Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

jsha Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

jsha left a comment

Choose a reason for hiding this comment

Uh oh!

jsha Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Feb 16, 2022

Uh oh!

jsha commented Feb 17, 2022

Uh oh!

bors commented Feb 17, 2022

Uh oh!

Uh oh!

GuillaumeGomez commented Feb 8, 2022 •

edited

Loading

GuillaumeGomez commented Feb 10, 2022 •

edited

Loading