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: clean up #toggle-all-docs #101944

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Sep 17, 2022

This change converts the element from an <a> link to a button. It's pretty much directly trading slightly more CSS for slightly less HTML, and it's also semantically correct (so you don't get a broken "bookmark" option when you right click on it).

While doing this, I also got rid of the unnecessary class="inner" attribute on the inner span. There was a style targeting .collapse-toggle > .inner, but no CSS ever targeted the #toggle-all-docs > .inner.

Preview: https://notriddle.com/notriddle-rustdoc-test/button-toggle-all-docs/index.html

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

r? @CraftSpider

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 17, 2022

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 Sep 17, 2022
@notriddle notriddle force-pushed the notriddle/toggle-all-docs branch from 8d05e40 to a7041e0 Compare September 17, 2022 16:42
@GuillaumeGomez
Copy link
Member

I think there was a reason why we kept a link here. Better check with @jsha first. If not, then it's a great cleanup! :)

@notriddle
Copy link
Contributor Author

@bors
Copy link
Contributor

bors commented Oct 8, 2022

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

@notriddle notriddle force-pushed the notriddle/toggle-all-docs branch from 3244967 to 0863f6c Compare October 24, 2022 18:02
@notriddle notriddle assigned jsha and unassigned CraftSpider Oct 24, 2022
This change converts the element from an `<a>` link to a button. It's
pretty much directly trading slightly more CSS for slightly less HTML, and
it's also semantically correct (so you don't get a broken "bookmark" option
when you right click on it).

While doing this, I also got rid of the unnecessary `class="inner"`
attribute on the inner span. There was a style targeting
`.collapse-toggle > .inner`, but no CSS ever targeted the
`#toggle-all-docs > .inner`.
@notriddle notriddle force-pushed the notriddle/toggle-all-docs branch from 0863f6c to bdbc977 Compare October 24, 2022 18:03
@notriddle notriddle closed this Oct 29, 2022
@notriddle notriddle deleted the notriddle/toggle-all-docs branch October 29, 2022 14:17
@GuillaumeGomez
Copy link
Member

You don't want to hear from @jsha first? Like I said, it's a great cleanup, I just wasn't sure if there wasn't a reason for this to be like that.

@notriddle notriddle restored the notriddle/toggle-all-docs branch October 29, 2022 14:29
@notriddle notriddle reopened this Oct 29, 2022
@jsha
Copy link
Contributor

jsha commented Oct 29, 2022

I didn't have any particular reason to keep this a link; I agree a button is better.

@GuillaumeGomez
Copy link
Member

Then let's approve it. Thanks @notriddle!

@bors r=jsha,GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Oct 30, 2022

📌 Commit bdbc977 has been approved by jsha,GuillaumeGomez

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 Oct 30, 2022
notriddle added a commit to notriddle/rust that referenced this pull request Oct 30, 2022
…s, r=jsha,GuillaumeGomez

rustdoc: clean up `#toggle-all-docs`

This change converts the element from an `<a>` link to a button. It's pretty much directly trading slightly more CSS for slightly less HTML, and it's also semantically correct (so you don't get a broken "bookmark" option when you right click on it).

While doing this, I also got rid of the unnecessary `class="inner"` attribute on the inner span. There was a style targeting `.collapse-toggle > .inner`, but no CSS ever targeted the `#toggle-all-docs > .inner`.

Preview: https://notriddle.com/notriddle-rustdoc-test/button-toggle-all-docs/index.html
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 31, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#97971 (Enable varargs support for calling conventions other than C or cdecl )
 - rust-lang#101428 (Add mir building test directory)
 - rust-lang#101944 (rustdoc: clean up `#toggle-all-docs`)
 - rust-lang#102101 (check lld version to choose correct option to disable multi-threading in tests)
 - rust-lang#102689 (Add a tier 3 target for the Sony PlayStation 1)
 - rust-lang#103746 (rustdoc: add support for incoherent impls on structs and traits)
 - rust-lang#103758 (Add regression test for reexports in search results)
 - rust-lang#103764 (All verbosity checks in `PrettyPrinter` now go through `PrettyPrinter::should_print_verbose`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8d6ed3e into rust-lang:master Oct 31, 2022
@rustbot rustbot added this to the 1.67.0 milestone Oct 31, 2022
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.

7 participants