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: remove old CSS selector that causes weird spacing #101335

Merged
merged 3 commits into from
Sep 3, 2022

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Sep 2, 2022

It was added with e08a84a (actually, it was called .methods > .stability at the time) and was directly nested that way.

EDIT: It is technically reachable code still, but it seems wrong.

With the old CSS rule still present

https://notriddle.com/notriddle-rustdoc-test/weird-spacing/lib/struct.Foo.html

image

Version 2 (an older version of this PR)

https://notriddle.com/notriddle-rustdoc-test/normal-spacing-2/lib/struct.Foo.html

image

Version 3 (with alignment fix for mobile)

https://notriddle.com/notriddle-rustdoc-test/normal-spacing-3/lib/struct.Foo.html

image

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Sep 2, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@rust-highfive
Copy link
Collaborator

r? @jsha

(rust-highfive has picked a reviewer for you, use r? to override)

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

Thanks! r=me once CI pass.

@notriddle notriddle closed this Sep 2, 2022
@notriddle
Copy link
Contributor Author

@GuillaumeGomez Never mind. I misunderstood some of the code (confused stability with stability_since).

Turns out you can still get this CSS reachable.

@notriddle notriddle reopened this Sep 2, 2022
@notriddle
Copy link
Contributor Author

@GuillaumeGomez However, I'm a bit curious if we actually want this CSS here. It still seems outdated, because when I manage to trigger it, the results look worse to me than if it were gone.

@GuillaumeGomez
Copy link
Member

If it creates ugly output when triggered, I'd say it's possible if it gets removed.

@GuillaumeGomez
Copy link
Member

r=me once CI pass then!

@notriddle
Copy link
Contributor Author

@GuillaumeGomez I posted some screenshots and links to samples. Do you think the "this is what we get if we merge this PR" version looks better than the old one?

@notriddle notriddle changed the title rustdoc: remove unused CSS selector .methods > .item-info rustdoc: remove old CSS selector that causes weird spacing Sep 2, 2022
It was added with e08a84a
(actually, it was called `.methods > .stability` at the time) and was
directly nested that way.

But with the switch to `<details>`, the code has changed drastically out from
under it, to the point where you have to go out of your way to actually get
it to render this way, and the result looks overly-tight and weird alongside
the normal version where this code is not reachable.
@GuillaumeGomez
Copy link
Member

Oh I see. So it has an impact when there is no doc block. Maybe we need to update:

.content .impl-items > .item-info {
    margin-left: 32px;
}

so they still are aligned? Adding a GUI test for it would be nice too.

@GuillaumeGomez
Copy link
Member

Can upload the updated version too please?

@notriddle
Copy link
Contributor Author

@GuillaumeGomez It's uploaded.

@GuillaumeGomez
Copy link
Member

Apart from the small potential improvement in the GUI test, looks great! 👍

@GuillaumeGomez
Copy link
Member

Thanks! r=me once CI pass

@notriddle
Copy link
Contributor Author

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 2, 2022

📌 Commit 0f29824 has been approved by notriddle

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 Sep 2, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 3, 2022
…ity, r=notriddle

rustdoc: remove old CSS selector that causes weird spacing

It was added with e08a84a (actually, it was called `.methods > .stability` at the time) and was directly nested that way.

**EDIT**: It is technically reachable code still, but it seems wrong.

## With the old CSS rule still present

https://notriddle.com/notriddle-rustdoc-test/weird-spacing/lib/struct.Foo.html

![image](https://user-images.githubusercontent.com/1593513/188216226-c667c560-d33d-494f-a492-4e0ec3ac0009.png)

## Version 2 (an older version of this PR)

https://notriddle.com/notriddle-rustdoc-test/normal-spacing-2/lib/struct.Foo.html

![image](https://user-images.githubusercontent.com/1593513/188216418-9fcd3109-f1b2-425d-b4fc-0c6b3b54e48e.png)

## Version 3 (with alignment fix for mobile)

https://notriddle.com/notriddle-rustdoc-test/normal-spacing-3/lib/struct.Foo.html

![image](https://user-images.githubusercontent.com/1593513/188223161-0e1ebce7-842f-41cb-8a0c-ae43aedcfccc.png)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 3, 2022
…ity, r=notriddle

rustdoc: remove old CSS selector that causes weird spacing

It was added with e08a84a (actually, it was called `.methods > .stability` at the time) and was directly nested that way.

**EDIT**: It is technically reachable code still, but it seems wrong.

## With the old CSS rule still present

https://notriddle.com/notriddle-rustdoc-test/weird-spacing/lib/struct.Foo.html

![image](https://user-images.githubusercontent.com/1593513/188216226-c667c560-d33d-494f-a492-4e0ec3ac0009.png)

## Version 2 (an older version of this PR)

https://notriddle.com/notriddle-rustdoc-test/normal-spacing-2/lib/struct.Foo.html

![image](https://user-images.githubusercontent.com/1593513/188216418-9fcd3109-f1b2-425d-b4fc-0c6b3b54e48e.png)

## Version 3 (with alignment fix for mobile)

https://notriddle.com/notriddle-rustdoc-test/normal-spacing-3/lib/struct.Foo.html

![image](https://user-images.githubusercontent.com/1593513/188223161-0e1ebce7-842f-41cb-8a0c-ae43aedcfccc.png)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2022
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#101335 (rustdoc: remove old CSS selector that causes weird spacing)
 - rust-lang#101347 (ffx component run should provide a collection)
 - rust-lang#101364 (Shrink suggestion span of argument mismatch error)
 - rust-lang#101365 (remove redundant clones)

Failed merges:

 - rust-lang#101349 (rustdoc: remove `.impl-items { flex-basis }` CSS, not in flex container)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5e34b79 into rust-lang:master Sep 3, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 3, 2022
@notriddle notriddle deleted the notriddle/methods-stability branch September 3, 2022 17:02
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.

6 participants