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: Fix badge alignment #105509

Closed

Conversation

GuillaumeGomez
Copy link
Member

It fixes a bug discovered by @joshtriplett.

It'll conflict with #105504 so I'll just rebase once the conflict arises. :)

r? @notriddle

@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 Dec 9, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@@ -766,6 +766,9 @@ table,
.item-left {
padding-right: 1.25rem;
}
.item-left > * {
vertical-align: middle;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use "middle" or "baseline"? It seems like the latter would work better given that the fonts may be somewhat different.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. They're guaranteed to be different, because Fira Sans is used for the link, while Source Code Serif is used for the badge.

  2. Not a fan of wildcard selectors. It's too tricky to figure out what they're actually supposed to target.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Middle:

Screenshot from 2022-12-10 18-22-10

Baseline:

image

I tend to prefer the "middle" alignment because "baseline" renders weirdly since even though they share the same baseline, they don't have the same height, making it a bit weird for the eye (imo). For "middle", they are vertically aligned which I find more pleasant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in short, I have a preference for the "middle" alignment but don't have a strong opinion about it so as is preferred.

@notriddle: I updated the selector to make it more specific.

@bors
Copy link
Contributor

bors commented Dec 10, 2022

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

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between a161a7b654083a881b22908a475988bcc3221a79 and 3736c4596d07c7a54bd53550527ec561a62f764d
Rustdoc was updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---
.......... (90/92)
..         (92/92)


/checkout/src/test/rustdoc-gui/stab-badge.goml stab-badge... FAILED
[ERROR] (line 6) Error: Evaluation failed: The following errors happened (for XPath `//*[@class='item-table']//*[@class='item-left module-item']/*[@class='stab deprecated']`): [expected `2px` for key `padding-top`, found `0px`]: for command `assert-css: (
    "//*[@class='item-table']//*[@class='item-left module-item']/*[@class='stab deprecated']",
    {"padding-top": "2px"},
)`
[ERROR] (line 10) Error: Evaluation failed: The following errors happened: [different Y values: 1891.390625 (or 1891) != 1892]: for command `assert-position: (
    "//*[@class='item-table']//*[@class='item-left module-item']/*[@class='stab deprecated']",
    {"y": 1892},
)`
[ERROR] (line 14) Error: Evaluation failed: The following errors happened: [different Y values: 1891.890625 (or 1892) != 1894]: for command `assert-position: (
    "//*[@class='item-table']//*[@class='item-left module-item']/*[@class='stab deprecated']/preceding-sibling::a",
    {"y": 1894}, // 1892 + 2 because of padding

Build completed unsuccessfully in 0:02:04

@anden3 anden3 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2023
@anden3
Copy link
Contributor

anden3 commented Apr 5, 2023

Hello @GuillaumeGomez! I just want to ping you as part of the triage procedure as this PR has merge conflicts and CI failures :)

@GuillaumeGomez
Copy link
Member Author

Seems like it needs to be discussed more so closing.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 23, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2023
Rollup merge of rust-lang#105666 - notriddle:notriddle/stab-baseline, r=GuillaumeGomez

rustdoc: align stability badge to baseline instead of bottom

| desc | img |
|------|-----|
| before | ![image](https://user-images.githubusercontent.com/1593513/207412598-3a6468ca-a169-4810-a689-4797688385df.png) |
| | |
| after | ![image](https://user-images.githubusercontent.com/1593513/207412720-b120269a-48a3-40e9-a9b0-6769bb05e104.png) |

Preview: http://notriddle.com/notriddle-rustdoc-demos/stab-baseline/test_dingus/index.html

Based on comment from rust-lang#105509 (comment)

r? ``@joshtriplett``
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

7 participants