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

rustdoc: merge theme css into rustdoc.css #115829

Merged
merged 2 commits into from
Sep 16, 2023
Merged

rustdoc: merge theme css into rustdoc.css #115829

merged 2 commits into from
Sep 16, 2023

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Sep 13, 2023 •
edited
Loading

Based on #115812 (comment)

Having them in separate files used to make more sense, before the migration to CSS variables made the theme files as small as they are nowadays. This is already how docs.rs and mdBook do it.

WebPageTest comparison page:

https://www.webpagetest.org/video/compare.php?tests=230913_AiDc3F_B9E,230913_AiDc7G_B9B

Filmstrip comparison:

image

Old waterfall:

image

New waterfall:

image

@rustbot
Copy link
Collaborator

rustbot commented Sep 13, 2023

r? @GuillaumeGomez

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 13, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 13, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

A change occurred in the Ayu theme.

cc @Cldfire

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@notriddle notriddle force-pushed the notriddle/rustdoc-theme-css-merge branch from 4a1f1ca to 5f9c3db Compare September 13, 2023 22:44
@rustbot
Copy link
Collaborator

rustbot commented Sep 13, 2023

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

A change occurred in the Ayu theme.

cc @Cldfire

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@notriddle notriddle force-pushed the notriddle/rustdoc-theme-css-merge branch from 5f9c3db to 07754de Compare September 13, 2023 23:17
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@notriddle notriddle force-pushed the notriddle/rustdoc-theme-css-merge branch 2 times, most recently from 5b8ba88 to 1970d25 Compare September 14, 2023 00:39
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@notriddle notriddle force-pushed the notriddle/rustdoc-theme-css-merge branch from 1970d25 to 0d626c7 Compare September 14, 2023 02:01
@GuillaumeGomez
Copy link
Member

I'm not too sure what to think about this change. The gain seems to be very small whereas now everything is in one file. The biggest downside being the rules for the ayu theme which all have to include to :root part. I'm not against it, just not sure if this is a good idea to go down this road completely.

An opinion maybe @jsha?

@notriddle
Copy link
Contributor Author

If it helps, this change not only improves first paint latency, but also removes a bunch of stuff from <head>, so it should reduce the size of the generated doc zipball.

@GuillaumeGomez
Copy link
Member

Oh that part I already figured out. It's really about maintenance. Also, did you check what happens if someone adds their own theme? Since we won't create the directory anymore, I'm curious how it goes if the folder doesn't exist.

@notriddle
Copy link
Contributor Author

Also, did you check what happens if someone adds their own theme?

There's already a run-make test that checks the directory layout. It works fine, because custom themes are placed at the doc root and have the resource suffix instead of a hash (like mytheme1.63.0.css).

There isn't a GUI test, though, and I managed to find a problem. It was fixable by making sure not to call document.write after the page is loaded (doing that blanks out the page).

@GuillaumeGomez
Copy link
Member

Ok, looks all good and square. So as said above, my only concern is about maintenance. I just want @jsha to have a look first before we move forward on this. Nice work in any case! 👍

@notriddle notriddle mentioned this pull request Sep 14, 2023
Merged
notriddle added a commit to notriddle/rust that referenced this pull request Sep 14, 2023
@notriddle
rusdoc: add gui test for custom CSS themes
cbccf80 
Based on
rust-lang#115829 (comment)
@notriddle notriddle force-pushed the notriddle/rustdoc-theme-css-merge branch from 0d626c7 to 68c0fc0 Compare September 14, 2023 20:29
@rustbot
Copy link
Collaborator

rustbot commented Sep 14, 2023

Some changes occurred in GUI tests.

cc @GuillaumeGomez

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2023
@bors
Auto merge of rust-lang#115857 - notriddle:notriddle/gui-test-themes,…
ca2b74f 
… r=GuillaumeGomez

rusdoc: add gui test for custom CSS themes

Based on
rust-lang#115829 (comment)
@jsha
Copy link
Contributor

jsha commented Sep 14, 2023

This looks good to me! Thanks for measuring the TTFP improvement, that'll be nice.

@GuillaumeGomez
Copy link
Member

Then let's go! Thanks again @notriddle.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 15, 2023

📌 Commit 68c0fc0 has been approved by GuillaumeGomez

It is now in the queue for this repository.

@notriddle notriddle force-pushed the notriddle/rustdoc-theme-css-merge branch from 68c0fc0 to ab41e2b Compare September 15, 2023 14:40
@notriddle
Copy link
Contributor Author

Windows paths in shell. Needed to put quotes around them in the run-make test to stop \ from being interpreted as escape codes.

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Sep 15, 2023

📌 Commit ab41e2b has been approved by 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 Sep 15, 2023
@bors
Copy link
Contributor

bors commented Sep 15, 2023

⌛ Testing commit ab41e2b with merge 81326e1...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2023
@bors
Auto merge of rust-lang#115829 - notriddle:notriddle/rustdoc-theme-cs…
81326e1 
…s-merge, r=GuillaumeGomez

rustdoc: merge theme css into rustdoc.css

Based on rust-lang#115812 (comment)

Having them in separate files used to make more sense, before the migration to CSS variables made the theme files as small as they are nowadays. This is already how docs.rs and mdBook do it.

WebPageTest comparison page:

https://www.webpagetest.org/video/compare.php?tests=230913_AiDc3F_B9E,230913_AiDc7G_B9B

Filmstrip comparison:

![image](https://github.com/rust-lang/rust/assets/1593513/7ccad27b-7497-47ee-94c0-1a701b69c0c2)

Old waterfall:

![image](https://github.com/rust-lang/rust/assets/1593513/7a6e4375-226d-4205-8871-a4d775a70748)

New waterfall:

![image](https://github.com/rust-lang/rust/assets/1593513/e29112e3-84f7-417d-a250-cd6c10fa50f5)
@bors
Copy link
Contributor

bors commented Sep 15, 2023

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 15, 2023
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@notriddle
Copy link
Contributor Author

@bors retry

x86_64-apple-1 fell over.

@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 15, 2023
@bors
Copy link
Contributor

bors commented Sep 16, 2023

⌛ Testing commit ab41e2b with merge c48e6ff...

@bors
Copy link
Contributor

bors commented Sep 16, 2023

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing c48e6ff to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 16, 2023
@bors bors merged commit c48e6ff into rust-lang:master Sep 16, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 16, 2023
This was referenced Sep 16, 2023
Merged
Closed
Merged
@notriddle notriddle deleted the notriddle/rustdoc-theme-css-merge branch September 16, 2023 03:02
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c48e6ff): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)		 -0.7% [-0.7%, -0.7%] 1
All ❌✅ (primary) -0.7% [-0.7%, -0.7%] 1

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.45s -> 633.302s (0.29%)
Artifact size: 318.06 MiB -> 318.18 MiB (0.04%)

@Nemo157 Nemo157 mentioned this pull request Mar 18, 2024
Merged
@ehuss ehuss mentioned this pull request Dec 8, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 9, 2024
@matthiaskrgr
Rollup merge of rust-lang#134045 - ehuss:triagebot-path-fixes, r=comp…
6f89a61 
…iler-errors

Fix some triagebot mentions paths

This fixes some mentions paths in `triagebot.toml` that are no longer valid.

* rustdoc themes were merged into rustdoc.css (which already has a mention, and unfortunately can't independently mention the ayu theme). rust-lang#115829 cc `@GuillaumeGomez` `@Cldfire`
* The entry for `inspect_obligations.rs` was never correct, from rust-lang#122385. cc `@lcnr` `@compiler-errors`
* The entry for `need_type_info.rs` was moved in rust-lang#127501
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2024
@rust-timer
Unrolled build for rust-lang#134045
2a052f1 
Rollup merge of rust-lang#134045 - ehuss:triagebot-path-fixes, r=compiler-errors

Fix some triagebot mentions paths

This fixes some mentions paths in `triagebot.toml` that are no longer valid.

* rustdoc themes were merged into rustdoc.css (which already has a mention, and unfortunately can't independently mention the ayu theme). rust-lang#115829 cc `@GuillaumeGomez` `@Cldfire`
* The entry for `inspect_obligations.rs` was never correct, from rust-lang#122385. cc `@lcnr` `@compiler-errors`
* The entry for `need_type_info.rs` was moved in rust-lang#127501
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 9, 2024
@matthiaskrgr
Rollup merge of #134045 - ehuss:triagebot-path-fixes, r=compiler-errors
75a5d7e 
Fix some triagebot mentions paths

This fixes some mentions paths in `triagebot.toml` that are no longer valid.

* rustdoc themes were merged into rustdoc.css (which already has a mention, and unfortunately can't independently mention the ayu theme). rust-lang/rust#115829 cc `@GuillaumeGomez` `@Cldfire`
* The entry for `inspect_obligations.rs` was never correct, from rust-lang/rust#122385. cc `@lcnr` `@compiler-errors`
* The entry for `need_type_info.rs` was moved in rust-lang/rust#127501
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@GuillaumeGomez GuillaumeGomez

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.
7 participants
@notriddle @rustbot @rust-log-analyzer @GuillaumeGomez @jsha @bors @rust-timer