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

Remove theme.js file #82732

Merged
merged 3 commits into from
Mar 23, 2021
Merged

Remove theme.js file #82732

merged 3 commits into from
Mar 23, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 3, 2021 •
edited
Loading

Fixes #82616.

The first commit moves the theme.js file into main.js, which requires to also run a small .replace on the main.js content.

The second commit is just a small cleanup to centralize DOM ids.

Since it removes a file from rustdoc output: cc @rust-lang/docs-rs

cc @jsha
r? @jyn514

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 3, 2021
@GuillaumeGomez GuillaumeGomez added A-rustdoc-ui Area: Rustdoc UI (generated HTML) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 3, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 3, 2021

r? @Nemo157

@rust-highfive rust-highfive assigned Nemo157 and unassigned jyn514 Mar 3, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jsha
Copy link
Contributor

jsha commented Mar 4, 2021

This looks good to me. One note: The first commit's summary is "Remove storage.js file creation and move its code inside main.js" but I think you meant to say theme.js.

@GuillaumeGomez
Copy link
Member Author

@jsha: Great catch, thanks! 😆

@Nemo157
Copy link
Member

Nemo157 commented Mar 4, 2021

It looks like main.js is loaded after the elements, does this cause a flash-of-unthemed-content on load because there is a paint before the theme is read from storage?

@GuillaumeGomez
Copy link
Member Author

This only changes the theme menu, not the picked theme itself (I made the same error haha). So it doesn't change anything from the rendering point of view.

Nemo157
Nemo157 reviewed Mar 4, 2021
View reviewed changes
src/librustdoc/html/static/main.js Outdated
@@ -83,12 +84,20 @@ function getSearchElement() {
return document.getElementById("search");
}

function getThemesElementId() {
Copy link
Member

@Nemo157 Nemo157 Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason this and getThemePickerElementId are functions instead of variableconstants?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's beause we can't use constant that I turned it into a function. But I can use a variable. Which do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

Script local variables seem better, to avoid thinking they need computing at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine by me!

@Nemo157
Copy link
Member

Nemo157 commented Mar 5, 2021

r=me with that last change

@jyn514 jyn514 mentioned this pull request Mar 5, 2021
Closed
@jyn514
Copy link
Member

jyn514 commented Mar 5, 2021 •
edited
Loading

Do you mind waiting for rust-lang/docs.rs#1302 to merge this? I hope to have it working by the end of the week, and it means less work for docs.rs.

@GuillaumeGomez
Copy link
Member Author

Sure, please r+ it once it's not blocked anymore then. ;)

@jyn514 jyn514 assigned jyn514 and unassigned Nemo157 Mar 5, 2021
@jyn514 jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 22, 2021

@bors r=Nemo157

@bors
Copy link
Contributor

bors commented Mar 22, 2021

📌 Commit 524fdf0 has been approved by Nemo157

@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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 22, 2021
@JohnTitor JohnTitor mentioned this pull request Mar 23, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2021
@bors
Auto merge of rust-lang#83398 - JohnTitor:rollup-om80krv, r=JohnTitor
2bd94f4 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#80705 (Update Source Code Pro and include italics)
 - rust-lang#81917 (Update RELEASES.md for 1.51.0)
 - rust-lang#82732 (Remove theme.js file)
 - rust-lang#83356 (rustdoc: Replace pair of `Option`s with an enum)
 - rust-lang#83384 (rename :pat2018 -> :pat2015)
 - rust-lang#83385 (:arrow_up: rust-analyzer)
 - rust-lang#83389 (add rust-analyzer rustc_private option in librustdoc Cargo.toml)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a34cc6b into rust-lang:master Mar 23, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 23, 2021
@GuillaumeGomez GuillaumeGomez deleted the remove-theme-file branch March 23, 2021 10:28
@jyn514 jyn514 mentioned this pull request Mar 25, 2021
Merged
@jyn514 jyn514 mentioned this pull request Apr 2, 2021
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nemo157 Nemo157 Nemo157 left review comments

Assignees

@jyn514 jyn514

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

Successfully merging this pull request may close these issues.

rustdoc: theme.js may be incorporated into main.js
8 participants
@GuillaumeGomez @rust-highfive @jyn514 @rust-log-analyzer @jsha @Nemo157 @bors @rustbot