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.

Sign up for GitHub

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

Jump to bottom

Remove "up here" arrow on item-infos #92651

Merged
merged 1 commit into from
Feb 6, 2022
Merged

Remove "up here" arrow on item-infos #92651

merged 1 commit into from
Feb 6, 2022

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jan 7, 2022

Use spacing to distinguish what is related to a given heading.

This was originally introduced in #53043, in response to #51387. The arrow is a little distracting, and leads the item-info to not be aligned properly with the text below it.

Demo: https://rustdoc.crud.net/jsha/impl-spacing/std/string/struct.String.html

r? @GuillaumeGomez

@jsha jsha 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 Jan 7, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2022
@Urgau
Copy link
Member

Urgau commented Jan 7, 2022

I like it but its a bit weird on top item; maybe it's because it's so off to the right.
image

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2022
@jsha
Copy link
Contributor Author

jsha commented Feb 1, 2022

Updated to pass tests and pushed a fresh demo. Note that I had to delete the "We collapse it" part of the hash-item-expansion test. It wasn't actually working the way it was supposed to- it was working somewhat accidentally because the display of details toggles is broken in the version of puppeteer bundled with browser-UI-test, and the moving around of margins made it break without a good possibility of fixing. The functionality tested - that clicking a toggle opens and closes it - is already adequately tested in other test files, and doesn't need to be specifically tested in hash-item-expansion, which is about making sure items are expanded when they're linked to.

@Urgau
Copy link
Member

Urgau commented Feb 1, 2022

@jsha Regarding my comment you could simply add margin-left: 0; here on #main-content > .item-info.

Before:
image

After:
image

@GuillaumeGomez
Copy link
Member

I like @Urgau's suggestion. (if you apply it, please add a GUI test for it)

@jsha
Remove "up here" arrow on item-infos
73d0f7c 
Use spacing to distinguish what is related to a given heading.
@jsha
Copy link
Contributor Author

jsha commented Feb 1, 2022

Done! Thanks for the suggestion, @Urgau.

@jsha
Copy link
Contributor Author

jsha commented Feb 4, 2022

I mention that in my last comment:

Note that I had to delete the "We collapse it" part of the hash-item-expansion test. It wasn't actually working the way it was supposed to- it was working somewhat accidentally because the display of details toggles is broken in the version of puppeteer bundled with browser-UI-test, and the moving around of margins made it break without a good possibility of fixing. The functionality tested - that clicking a toggle opens and closes it - is already adequately tested in other test files, and doesn't need to be specifically tested in hash-item-expansion, which is about making sure items are expanded when they're linked to.

@GuillaumeGomez
Copy link
Member

Sorry, missed it. Then all good for me, thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Feb 5, 2022

@GuillaumeGomez: 🔑 Insufficient privileges: Not in reviewers

@GuillaumeGomez
Copy link
Member

Weird... Let's retry.

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 5, 2022

📌 Commit 73d0f7c has been approved by GuillaumeGomez

@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 Feb 5, 2022
@GuillaumeGomez
Copy link
Member

cc @rust-lang/infra ^

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2022
@matthiaskrgr
Rollup merge of rust-lang#92651 - jsha:impl-spacing, r=GuillaumeGomez
62fb88e 
Remove "up here" arrow on item-infos

Use spacing to distinguish what is related to a given heading.

This was originally introduced in rust-lang#53043, in response to rust-lang#51387. The arrow is a little distracting, and leads the item-info to not be aligned properly with the text below it.

Demo: https://rustdoc.crud.net/jsha/impl-spacing/std/string/struct.String.html

r? `@GuillaumeGomez`
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 5, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 6, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2022
@bors
Auto merge of rust-lang#93689 - matthiaskrgr:rollup-3pd1ept, r=matthi…
e069a71 
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#91939 (Clarify error on casting larger integers to char)
 - rust-lang#92300 (mips64-openwrt-linux-musl: Add Tier 3 target)
 - rust-lang#92383 (Add new target armv7-unknown-linux-uclibceabi (softfloat))
 - rust-lang#92651 (Remove "up here" arrow on item-infos)
 - rust-lang#93556 (Change struct expr pretty printing to match rustfmt style)
 - rust-lang#93649 (Add regression tests for issue 80309)
 - rust-lang#93657 (Update CPU idle tracking for apple hosts)
 - rust-lang#93659 (Refactor conditional)
 - rust-lang#93669 (Resolve lifetimes for const generic defaults)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 05bb32d into rust-lang:master Feb 6, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 6, 2022
@jsha jsha mentioned this pull request Feb 8, 2022
Merged
@jsha jsha deleted the impl-spacing branch February 8, 2022 09:22
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 9, 2022
@matthiaskrgr
Rollup merge of rust-lang#93763 - jsha:re-space-empty-impls, r=Guilla…
2184cf7 
…umeGomez

rustdoc: fix spacing of non-toggled impl blocks

We [recently removed the "up here" arrows on item-infos](rust-lang#92651), and adjusted
vertical spacing so that even without the arrow, it would be visually
clear which item the item-info belonged to. The new CSS styles for
vertical spacing only applied to toggles, though. This missed
non-toggled impl blocks - for instance, those without any methods, like
https://doc.rust-lang.org/nightly/std/marker/trait.Send.html#implementors.
The result was lists of implementors that were spaced too closely. This
PR fixes the spacing by making it apply to non-toggled impl blocks as
well.

This also fixes an issue where item-infos were displayed too far below
their items. That was a result of display: table on .item-info .stab.
Changed that to display: inline-block.

Demo: https://rustdoc.crud.net/jsha/re-space-empty-impls/std/marker/trait.Send.html

Before:

<img width=300 src="https://user-images.githubusercontent.com/220205/152954394-ec0b80e7-2573-4f06-9d7a-7b10b8ceac60.png">

After:

<img width=300 src="https://user-images.githubusercontent.com/220205/152954228-abac1d30-a76d-4ab1-89ec-ef7549fe8c9c.png">

r? `@GuillaumeGomez`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2022
@bors
Auto merge of rust-lang#93763 - jsha:re-space-empty-impls, r=Guillaum…
c26fbf8 
…eGomez

rustdoc: fix spacing of non-toggled impl blocks

We [recently removed the "up here" arrows on item-infos](rust-lang#92651), and adjusted
vertical spacing so that even without the arrow, it would be visually
clear which item the item-info belonged to. The new CSS styles for
vertical spacing only applied to toggles, though. This missed
non-toggled impl blocks - for instance, those without any methods, like
https://doc.rust-lang.org/nightly/std/marker/trait.Send.html#implementors.
The result was lists of implementors that were spaced too closely. This
PR fixes the spacing by making it apply to non-toggled impl blocks as
well.

This also fixes an issue where item-infos were displayed too far below
their items. That was a result of display: table on .item-info .stab.
Changed that to display: inline-block.

Demo: https://rustdoc.crud.net/jsha/re-space-empty-impls/std/marker/trait.Send.html

Before:

<img width=300 src="https://user-images.githubusercontent.com/220205/152954394-ec0b80e7-2573-4f06-9d7a-7b10b8ceac60.png">

After:

<img width=300 src="https://user-images.githubusercontent.com/220205/152954228-abac1d30-a76d-4ab1-89ec-ef7549fe8c9c.png">

r? `@GuillaumeGomez`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
8 participants
@jsha @rust-highfive @Urgau @rust-log-analyzer @GuillaumeGomez @bors @JohnCSimon @rustbot