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

rustdoc: include link on all.html location header #108686

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

notriddle
Copy link
Contributor

This avoids a subtle layout shift when switching from the crate page to all items.

Before

index.html all.html
image image

After

index.html all.html
image image

@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2023

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 3, 2023
@rust-log-analyzer

This comment has been minimized.

@jsha
Copy link
Contributor

jsha commented Mar 4, 2023

Nice catch. Generally looks good, just two questions:

  • It's odd that the layout of this h2 is affected by whether it contains an <a>. That suggests the dimensions of our h2.location are being determined by their nested <a> rather than the <h2> itself; it would be tidy to give rules to h2.location that make it right-sized on its own.

  • On most pages, the h2.location link takes you to the top of the current page. In this PR, the h2.location link takes you to the crate root. I think consistency is better here - taking you to the top of the page, even though the actual title says "Crate std". If someone wants to go to the crate root, the logo will do that.

@notriddle
Copy link
Contributor Author

  1. That’s because the link itself has padding. We want the padding to be part of the link so that it acts as part of the click target.
  2. Sure, I can do that.

@notriddle
Copy link
Contributor Author

@jsha Okay, it's been switched to #

This avoids a subtle layout shift when switching from the crate page
to all items.
@jsha
Copy link
Contributor

jsha commented Mar 8, 2023

@bors r+ rollup

/cc @clubby789 FYI this will introduce a small merge conflict for #108784. Although I suspect #108784 also happens to fix this by making the <h2 class="location"> always contain an <a>. Still it's nice to have the test that's in this PR.

@bors
Copy link
Contributor

bors commented Mar 8, 2023

📌 Commit af664be has been approved by jsha

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#108686 (rustdoc: include link on all.html location header)
 - rust-lang#108846 (StableMIR: Proof-of-concept implementation + test )
 - rust-lang#108873 (Simplify `sort_by` calls)
 - rust-lang#108883 (Suppress copy impl error when post-normalized type references errors)
 - rust-lang#108884 (Tweak illegal `Copy` impl message)
 - rust-lang#108887 (Rename `MapInPlace` as `FlatMapInPlace`.)
 - rust-lang#108901 (fix: evaluate with wrong obligation stack)
 - rust-lang#108903 (Move Clippy tests back to their intended directory)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6c91ce2 into rust-lang:master Mar 9, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 9, 2023
@notriddle notriddle deleted the notriddle/jank-all branch March 9, 2023 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants