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

<details>/<summary> UI fixes #97249

Merged
merged 3 commits into from
Jul 2, 2022
Merged

<details>/<summary> UI fixes #97249

merged 3 commits into from
Jul 2, 2022

Conversation

GuillaumeGomez
Copy link
Member

With images it's easier to understand:

Screenshot from 2022-05-21 14-10-42
Screenshot from 2022-05-21 14-08-49

The headings in <summary> should not have bottom border so I removed it as well alongside the other fixes.

r? @jsha

@GuillaumeGomez GuillaumeGomez 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 May 21, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd,@jsha

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2022
@bilelmoussaoui bilelmoussaoui mentioned this pull request May 21, 2022
Merged
@GuillaumeGomez GuillaumeGomez force-pushed the details-summary-fixes branch from a31048c to 12c293e Compare May 24, 2022 11:24
@bors
Copy link
Contributor

bors commented May 26, 2022

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

@GuillaumeGomez GuillaumeGomez force-pushed the details-summary-fixes branch from 12c293e to bfd7d16 Compare May 26, 2022 14:03
@GuillaumeGomez
Copy link
Member Author

Fixed merge conflicts.

@jsha
Copy link
Contributor

jsha commented May 29, 2022

Can you explain this bug more? It sounds like the problem is: doc authors sometimes write <details><summary> in their docs, and when they do, the rustdoc styles are inappropriately applying to those. Is that right?

If so, I think the solution is to make the rustdoc styles more selective rather than to re-override them with other styles. In particular the content: "▶"; and content: "▼"; properties are suspicious. We should allow the browser styles to come through in those cases rather than try to imitate the browser styles with our own ad-hoc icons.

@GuillaumeGomez
Copy link
Member Author

Can you explain this bug more? It sounds like the problem is: doc authors sometimes write <details><summary> in their docs, and when they do, the rustdoc styles are inappropriately applying to those. Is that right?

Yes, it's exactly that! Sorry, should have explained more alongside the picture.

If so, I think the solution is to make the rustdoc styles more selective rather than to re-override them with other styles. In particular the content: "▶"; and content: "▼"; properties are suspicious. We should allow the browser styles to come through in those cases rather than try to imitate the browser styles with our own ad-hoc icons.

Good point. For some reason the toggle was hidden but I'll investigate a bit more and try to remove the need for this CSS.

@GuillaumeGomez GuillaumeGomez force-pushed the details-summary-fixes branch from bfd7d16 to 5a57639 Compare May 30, 2022 14:31
@GuillaumeGomez GuillaumeGomez force-pushed the details-summary-fixes branch from 5a57639 to b8db8cc Compare May 30, 2022 14:38
@GuillaumeGomez
Copy link
Member Author

I now know why I needed the create the marker with the CSS: if the display isn't list-items, then it hides the marker on firefox. So instead, I change the display property of summary children.

@notriddle
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 1, 2022

📌 Commit b8db8cc has been approved by notriddle

@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 Jul 1, 2022
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jul 1, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 1, 2022
@bors
Auto merge of rust-lang#98781 - GuillaumeGomez:rollup-798kb8u, r=Guil…
9a6fa4f 
…laumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#97249 (`<details>`/`<summary>` UI fixes)
 - rust-lang#98418 (Allow macOS to build LLVM as shared library)
 - rust-lang#98460 (Use CSS variables to handle theming)
 - rust-lang#98497 (Improve some inference diagnostics)
 - rust-lang#98708 (rustdoc: fix 98690 Panic if invalid path for -Z persist-doctests)

Failed merges:

 - rust-lang#98761 (more `need_type_info` improvements)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 194764f into rust-lang:master Jul 2, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 2, 2022
@GuillaumeGomez GuillaumeGomez deleted the details-summary-fixes branch July 2, 2022 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jsha jsha

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.64.0
Development

Successfully merging this pull request may close these issues.
6 participants
@GuillaumeGomez @rust-highfive @bors @jsha @notriddle @rustbot