-
Notifications
You must be signed in to change notification settings - Fork 13k
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 missing bottom border for headings in sidebar #90571
Fix missing bottom border for headings in sidebar #90571
Conversation
Some changes occurred in HTML/CSS/JS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching and fixing this!
@@ -156,7 +156,9 @@ h1.fqn > .in-band > a:hover { | |||
section hierarchies. */ | |||
h2, | |||
.top-doc h3, | |||
.top-doc h4 { | |||
.top-doc h4, | |||
.sidebar .sidebar-elems h3:not(.sidebar-title), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the not
selectors are pretty hard to maintain and reason about. Can we do this with a positive selector instead? For instance, it looks like the headings we need underlines on here are nested under .block
, so maybe we could do .sidebar .block h3
?
Also it's not clear to me why both .sidebar
and .sidebar-elems
are needed in this selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except that the .sidebar-title
are in .block
as well. :3
It was more complicated than I expected to actually add this rule without breaking something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about selecting the specific block types that get the underline treatment? Like:
.sidebar .block.struct h3, .sidebar .block.trait h3, .sidebar .block.type, ... {
Alternately, we could change the HTML structure slightly so everything after "Other items in ..." is nested under a specific class, like .other-items
. Then we could make this selector .sidebar .other-items h3
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to have a CSS class in the generated HTML, seems much simpler.
9db489e
to
0b04b8e
Compare
@jsha It now looks much better, thanks for the suggestion! |
Looks great. r=me once CI passes |
This comment has been minimized.
This comment has been minimized.
0b04b8e
to
aa17e1c
Compare
@bors: r=jsha rollup |
📌 Commit aa17e1c has been approved by |
…r-sidebar, r=jsha Fix missing bottom border for headings in sidebar Fixes rust-lang#90568. r? `@jsha`
…r-sidebar, r=jsha Fix missing bottom border for headings in sidebar Fixes rust-lang#90568. r? ``@jsha``
Rollup of 9 pull requests Successful merges: - rust-lang#90507 (Suggest `extern crate alloc` when using undeclared module `alloc`) - rust-lang#90530 (Simplify js tester a bit) - rust-lang#90533 (Add note about x86 instruction prefixes in asm! to unstable book) - rust-lang#90537 (Update aarch64 `target_feature` list for LLVM 12.) - rust-lang#90544 (Demote metadata load warning to "info".) - rust-lang#90554 (Clean up some `-Z unstable-options` in tests.) - rust-lang#90556 (Add more text and examples to `carrying_{add|mul}`) - rust-lang#90563 (rustbot allow labels) - rust-lang#90571 (Fix missing bottom border for headings in sidebar) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #90568.
r? @jsha