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

Cleanup rustdoc themes #100494

Merged
merged 3 commits into from
Aug 19, 2022
Merged

Cleanup rustdoc themes #100494

merged 3 commits into from
Aug 19, 2022

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Aug 13, 2022

This PR continues our work to simplify the rustdoc themes by relying more on CSS variables. Interestingly enough, this time it allowed me to realize that we were having a lot of different colors for borders even though the difference is unnoticeable. I used this opportunity to unify them.

Follow-up of #98460.

The live demo is here.

r? @jsha

@GuillaumeGomez GuillaumeGomez added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-themes Area: Themes for HTML pages generated by rustdoc A-rustdoc-ui Area: rustdoc UI (generated HTML) labels Aug 13, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 13, 2022

A change occurred in the Ayu theme.

cc @Cldfire

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2022
src/librustdoc/html/static/css/rustdoc.css Show resolved Hide resolved
Comment on lines 980 to 983
/* Without the `!important`, the border-color is ignored for `<select>`...
It cannot be in the group above because `.search-input` has a different border color on
hover. */
border: 1px solid var(--border-color) !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment. I think border: 1px solid var(--border-color) should work.

Are you familiar with how CSS Specificity works? Most instances of !important can be better solved by applying the principles of specificity.

Also, when you say "cannot be in the group above", (a) you should say "in the rule above" or "in the ruleset above" (https://developer.mozilla.org/en-US/docs/Web/CSS/Syntax#css_rulesets). Using precise language helps clarify. Also: are you talking about the ruleset with the selector #crate-search-div? I don't see how that one is relevant.

src/librustdoc/html/static/css/rustdoc.css Outdated Show resolved Hide resolved
src/librustdoc/html/static/css/themes/ayu.css Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

I also updated the online demo.

@GuillaumeGomez
Copy link
Member Author

Added the border everywhere and removed it from ayu theme.

@@ -138,6 +138,9 @@ h1, h2, h3, h4 {
.docblock h3, .docblock h4, h5, h6 {
margin: 15px 0 5px 0;
}
.docblock table td, .docblock table th {
border-color: var(--border-color);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be combined with the rules that set the border for these elements in the first place.

.docblock table td {
	padding: .5em;
	border: 1px dashed;
}

.docblock table th {
	padding: .5em;
	text-align: left;
	border: 1px solid;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes absolutely. Don't know how I didn't think about it...

}
#settings-menu > a, #help-button > button, #copy-path {
padding: 5px;
width: 33px;
border: 1px solid;
border-radius: 2px;
cursor: pointer;
border-color: var(--border-color);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be combined with the border property on line 1543 above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm quite ashamed of this one...

@@ -1539,6 +1552,7 @@ pre.rust {
padding: 5px;
height: 100%;
display: block;
background-color: var(--button-background-color);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not needed. It's redundant with the ruleset at line 1540.

@@ -840,11 +848,11 @@ nav.main {
text-align: center;
}
nav.main .current {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this ruleset entirely. We don't currently generate a nav.main .current AFAICT.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't seem to have a nav.main indeed (but we do have .current) so this rule is unused.

@GuillaumeGomez
Copy link
Member Author

Applied suggestions and updated live demo.

@jsha
Copy link
Contributor

jsha commented Aug 18, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 18, 2022

📌 Commit 09396fc has been approved by jsha

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 Aug 18, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#99576 (Do not allow `Drop` impl on foreign fundamental types)
 - rust-lang#100081 (never consider unsafe blocks unused if they would be required with deny(unsafe_op_in_unsafe_fn))
 - rust-lang#100208 (make NOP dyn casts not require anything about the vtable)
 - rust-lang#100494 (Cleanup rustdoc themes)
 - rust-lang#100522 (Only check the `DefId` for the recursion check in MIR inliner.)
 - rust-lang#100592 (Manually implement Debug for ImportKind.)
 - rust-lang#100598 (Don't fix builtin index when Where clause is found)
 - rust-lang#100721 (Add diagnostics lints to `rustc_type_ir` module)
 - rust-lang#100731 (rustdoc: count deref and non-deref as same set of used methods)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3cebcba into rust-lang:master Aug 19, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 19, 2022
@GuillaumeGomez GuillaumeGomez deleted the cleanup-themes branch August 20, 2022 08:59
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 3, 2022
…r=notriddle

Cleanup css theme

Follow-up of rust-lang#100494.

The change for the border color of the search input in the dark mode was actually a weird case: the search input border was unique, it didn't share the same variable with other items with borders. This weird case being unique to the dark theme, I removed it, hence the modification in the GUI test.

Live demo is [here](https://rustdoc.crud.net/imperio/cleanup-css-theme/std/index.html).

cc `@jsha`
r? `@notriddle`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-themes Area: Themes for HTML pages generated by rustdoc A-rustdoc-ui Area: rustdoc UI (generated HTML) C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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