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: clarify theme settings #84539

Closed
jsha opened this issue Apr 25, 2021 · 11 comments · Fixed by #92629
Closed

rustdoc: clarify theme settings #84539

jsha opened this issue Apr 25, 2021 · 11 comments · Fixed by #92629
Labels
A-rustdoc-js Area: Rustdoc's JS front-end C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jsha
Copy link
Contributor

jsha commented Apr 25, 2021

Right now, you can set theme options in two places, the paintbrush icon, and the settings page:

image

It's not clear how these are related. Does one override the other? Also, what is the "system theme"?

I propose that we move the paintbrush icon's functionality into the settings page, and overhaul the settings available there.

GitHub has put a lot of attention into their "dark mode" implementation and might make a good example for verbiage:


image


image


Following that example, the settings page should have a choice between "single theme" and "sync with system", with a single dropdown visible if you pick "single theme," and two dropdowns (for light/dark) visible if you pick "sync with system."

Prior work:

#77213
#79642
#77809
#77213

@jsha jsha added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-rustdoc-js Area: Rustdoc's JS front-end T-rustdoc labels Apr 25, 2021
@camelid camelid added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 28, 2021
@jyn514 jyn514 removed the T-rustdoc label May 7, 2021
@GuillaumeGomez
Copy link
Member

That would be problematic when browsing documentation from the filesystem and not with a server because then, it'd not be possible to change the theme because the local storage is stored at a folder level.

@jsha
Copy link
Contributor Author

jsha commented Sep 30, 2021

One way we could address that issue would be to make the settings page rendered by JS instead of a standalone HTML page. That would also allow all the other settings to be applied in the file:/// context. Another way would be to decide that certain features (like theme switching) only work locally if you use, e.g. `python -m http.server' so you can access the files at an HTTP URL.

@GuillaumeGomez
Copy link
Member

That could work I guess... I'm not super happy about adding more JS. Also, currently it's very easy to change the current theme, putting it into the setting page would make this more confusing I think.

@camelid
Copy link
Member

camelid commented Oct 26, 2021

I left a related comment about this here.

@Nemo157
Copy link
Member

Nemo157 commented Oct 27, 2021

That would be problematic when browsing documentation from the filesystem and not with a server because then, it'd not be possible to change the theme because the local storage is stored at a folder level.

Doesn't this just mean that changing the theme is already broken when browsing via file:/// (along with all the other settings)? I just tried it out and if you change theme in a few places you essentially get it just randomly jumping around when you go to different modules since they all have their own local storage settings.

@jsha
Copy link
Contributor Author

jsha commented Oct 27, 2021

Doesn't this just mean that changing the theme is already broken when browsing via file:/// (along with all the other settings)?

It depends on the browser. On Firefox the storage appears to be isolated per-file (in other words, yes, theme selection is broken on navigation between local files). On Chrome it seems to persist across various files. That would mean putting theme settings on the settings page would work fine on Chrome, but on Firefox it would further break the already-broken behavior. If you're in Firefox viewing a single page repeatedly, right now you can set the theme for that single page even though it won't take effect for neighboring pages.

I don't think there's anything we can do to get Firefox to persist settings between different file:/// URLs.

@GuillaumeGomez
Copy link
Member

To be more precise, on firefox it's per-folder and not per-file.

I don't think there's anything we can do to get Firefox to persist settings between different file:/// URLs.

I can confirm that there isn't anything which can be done... If only we could tell firefox "this is the root path so apply for all children"...

So any JS-based solution won't work on firefox, and on chrome it's not much better iirc.

@jsha
Copy link
Contributor Author

jsha commented Oct 30, 2021

To be more precise, on firefox it's per-folder and not per-file.

In my local tests (Firefox 93.0, Linux), it's per-file. Steps to reproduce:

  1. Navigate to build/x86_64-unknown-linux-gnu/doc/std/string/struct.String.html
  2. Set theme to dark
  3. Navigate to build/x86_64-unknown-linux-gnu/doc/std/string/struct.Drain.html
  4. Notice theme is not dark. But hitting back and reload shows the theme is still dark on the String page.

So is it okay by you moving the theme setting onto the settings page? Things would continue to work as they do today on Chrome. On Firefox, you would stop being able to set the theme page-at-a-time, but I think the current page-at-a-time functionality on Firefox is not very useful.

@GuillaumeGomez
Copy link
Member

I'm pretty sure that if we do that, people will simply complain because they can't change the theme anymore. And that would also prevent to have theme change which I quite enjoy locally.

As for your scenario, I tried it locally but it worked fine. Weird... I don't seem to have any specific customization though...

@GuillaumeGomez
Copy link
Member

From MDN:

For documents loaded from file: URLs (that is, files opened in the browser directly from the user’s local filesystem, rather than being served from a web server) the requirements for localStorage behavior are undefined and may vary among different browsers.

Undefined, great.

@jsha
Copy link
Contributor Author

jsha commented Jan 6, 2022

@GuillaumeGomez and I discussed via PM on Zulip, and came to a compromise solution: We'll show the theme picker icon only when visiting a file:/// URL. That way we get a nice uncluttered UI on doc.rust-lang.org and docs.rs, but we don't lose the theme-picking functionality on local files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-js Area: Rustdoc's JS front-end C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants