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

Switch settings menu to full js #93097

Merged
merged 4 commits into from
May 3, 2022
Merged

Switch settings menu to full js #93097

merged 4 commits into from
May 3, 2022

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 19, 2022 •
edited
Loading

Since the settings can only be set when the JS is enabled, it's not really a problem. It also fixes a debate we had around the themes not being accessible easily before.

Screenshot from 2022-01-19 23-06-59

You can test it here.

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) A-rustdoc-js Area: Rustdoc's JS front-end labels Jan 19, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 2022
@jsha
Copy link
Contributor

jsha commented Jan 19, 2022

Some early feedback from testing: This should generate a page navigation event, like searching does, so that when you hit the "back" button in your browser you go back to the page you were on.

The page navigation event should make the URL /settings.html, and we could keep an actual settings.html that just calls the JS to generate the page contents.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

The page navigation event should make the URL /settings.html, and we could keep an actual settings.html that just calls the JS to generate the page contents.

That'd mean that we'd need to have a settings.html file in every folder, no? Maybe simpler to keep it like search with a query parameter, don't you think?

jsha
jsha requested changes Jan 19, 2022
View reviewed changes
src/librustdoc/templates/page.html Outdated Show resolved Hide resolved
@jsha
Copy link
Contributor

jsha commented Jan 19, 2022

That'd mean that we'd need to have a settings.html file in every folder, no? Maybe simpler to keep it like search with a query parameter, don't you think?

No, I'm proposing to have one settings.html per documentation instance, just like we have today - and in exactly the same place.

jsha
jsha requested changes Jan 19, 2022
View reviewed changes
src/librustdoc/html/static/js/main.js Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

No, I'm proposing to have one settings.html per documentation instance, just like we have today - and in exactly the same place.

I'm not a big fan of the idea: it means duplicating where the settings are set and generated... I think it's extra work for not much gain.

@jsha
Copy link
Contributor

jsha commented Jan 19, 2022

it means duplicating where the settings are set and generated... I think it's extra work for not much gain.

I think you misunderstand me. I'm not proposing that we should generate the settings into HTML. I'm proposing that we have HTML that is an empty shell, and just calls loadScript(window.settingsJS);. The settingsJS is still responsible for rendering the settings via JS.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

it means duplicating where the settings are set and generated... I think it's extra work for not much gain.

I think you misunderstand me. I'm not proposing that we should generate the settings into HTML. I'm proposing that we have HTML that is an empty shell, and just calls loadScript(window.settingsJS);. The settingsJS is still responsible for rendering the settings via JS.

Oh indeed. Great idea!

@bors
Copy link
Contributor

bors commented Jan 20, 2022

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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jsha jsha mentioned this pull request Jan 23, 2022
Closed
@bors
Copy link
Contributor

bors commented Jan 23, 2022

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

@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 18, 2022
Closed
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2022
@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 4, 2022
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jsha
Copy link
Contributor

jsha commented Apr 28, 2022

This needs another merge deconflict.

@jsha
Copy link
Contributor

jsha commented Apr 28, 2022

From the demo:

image

Two problems:

  • This says "preferred-dark-theme" etc, but should say "Preferred dark theme"
  • The "Preferred light theme" setting should come ahead of the "Preferred dark theme" as it currently does.

jsha
jsha requested changes Apr 28, 2022
View reviewed changes
src/librustdoc/html/static/js/settings.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/settings.js Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Thanks for the review! I'll update it tomorrow.

@GuillaumeGomez
Copy link
Member Author

Updated!

@bors
Copy link
Contributor

bors commented Apr 30, 2022

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

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflicts. :)

@jsha
Copy link
Contributor

jsha commented May 1, 2022

This still has "Preferred dark theme" above "Preferred light theme." It should be the other way around, to match the current UI.

jsha
jsha requested changes May 1, 2022
View reviewed changes
src/librustdoc/html/static/js/main.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/main.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/settings.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/settings.js Outdated Show resolved Hide resolved
@GuillaumeGomez
* Add documentation for settings page rendering functions.
73688e4 
* Improve code.
* Fix some documentation argument types.
* Make settings order the same as before this PR.
* Change timeout to 0 so that browser will render it as fast as possible.
@GuillaumeGomez
Copy link
Member Author

Updated and uploaded updated version. :)

@jsha
Copy link
Contributor

jsha commented May 2, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 2, 2022

📌 Commit 73688e4 has been approved by jsha

@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 May 2, 2022
@GuillaumeGomez
Copy link
Member Author

🎉

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 2, 2022
@GuillaumeGomez
Rollup merge of rust-lang#93097 - GuillaumeGomez:settings-js, r=jsha
60b238b 
Switch settings menu to full js

Since the settings can only be set when the JS is enabled, it's not really a problem. It also fixes a debate we had around the themes not being accessible easily before.

![Screenshot from 2022-01-19 23-06-59](https://user-images.githubusercontent.com/3050060/150221936-fd1a1e76-06b6-4416-a653-dbae111979ed.png)

You can test it [here](https://rustdoc.crud.net/imperio/settings-js/doc/foo/index.html).

r? `@jsha`
@GuillaumeGomez GuillaumeGomez mentioned this pull request May 2, 2022
Closed
@JohnTitor JohnTitor mentioned this pull request May 3, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2022
@bors
Auto merge of rust-lang#96663 - JohnTitor:rollup-lthuzdq, r=JohnTitor
468492c 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#93097 (Switch settings menu to full js)
 - rust-lang#96587 (Refactor the WriteBackendMethods and ExtraBackendMethods traits)
 - rust-lang#96589 (Use source callsite in check_argument_types suggestion)
 - rust-lang#96599 (Update `RValue::Discriminant` documentation)
 - rust-lang#96614 (Add a regression test for rust-lang#92305)
 - rust-lang#96629 (Fix invalid keyword order for function declarations)
 - rust-lang#96641 (Use a yes/no enum instead of a bool.)
 - rust-lang#96646 (Mitigate impact of subtle invalid call suggestion logic)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 27d7615 into rust-lang:master May 3, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 3, 2022
@GuillaumeGomez GuillaumeGomez deleted the settings-js branch May 3, 2022 09:22
@notriddle notriddle mentioned this pull request Oct 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notriddle notriddle notriddle left review comments

@jsha jsha jsha requested changes

Assignees

@jsha jsha

Labels
A-rustdoc-js Area: Rustdoc's JS front-end 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.62.0
Development

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