-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Pick themes on settings page, not every page #92629
Conversation
Some changes occurred in HTML/CSS/JS. |
This comment has been minimized.
This comment has been minimized.
My thought was that people would use the "preferred dark theme" / "preferred light theme". But we also need something on the settings page for when "Use system theme" is off. I'll work on that. |
1e1b20a
to
9bbf017
Compare
Ok, updated the settings page too. |
This comment has been minimized.
This comment has been minimized.
Showing the paintbrush icon on
|
"preferred theme" seems weirdly worded to me. Also, when changing its value, it doesn't change the current theme. @notriddle: Interestingly enough, you have the exact opposite logic that we used. When on a website, you only need to change it once to get it applied correctly on all the pages whereas it's not the case for local docs (hence why we keep it there).
I'm not sure to see how much of an issue it is. Could you elaborate a bit more?
I agree that it's not the best. The idea is to reduce the number of buttons we have because they're distracting (@jsha received complains about it iirc) when it's not really necessary (because again, on a website you only need to change it once). An alternative would be for the settings to be 100% JS and therefore to have access to it on all pages, allowing us to remove tje theme button on all pages. But that bring other issues. |
If the theme gets “randomly” reverted when you switch pages, is the functionality really useful? Blinding white flashes in dark mode can be pretty uncomfortable. Personally, I stopped setting rustdoc themes, exactly because it doesn’t really work.
Since the image is only hidden by CSS, the SVG file still gets downloaded. There’s also quite a bit of CSS and JavaScript used to implement that control, since it’s currently the only JavaScript-based drop-down in all of rustdoc. If it was removed, all that code could go away, or be migrated to settings.js and not get loaded by default. |
☔ The latest upstream changes (presumably #92526) made this pull request unmergeable. Please resolve the merge conflicts. |
9bbf017
to
31e7f23
Compare
This comment has been minimized.
This comment has been minimized.
This hides the paintbrush icon on most pages by default, in preference for the settings on the settings page. When loading from a local file, and not in mobile view, continue to show the theme picker. That's because some browsers limit access to localStorage from file:/// URLs, so choosing a theme from settings.html doesn't take effect.
31e7f23
to
692f9b6
Compare
This comment has been minimized.
This comment has been minimized.
692f9b6
to
04f0402
Compare
The display of the setting of the theme selection is still not great. Do you have an idea on how improve it? (If you have ideas for the whole setting page, go ahead! :D ) |
I have some ideas about improving the design of the settings page in general, but I don't think they should block progress on this issue. The goal of the settings change in this PR is to make sure the settings page has the same features as the paintbrush, so we can remove the paintbrush in most cases. The design is the same as it is on https://doc.rust-lang.org/nightly/settings.html. |
📌 Commit 04f0402 has been approved by |
…askrgr Rollup of 14 pull requests Successful merges: - rust-lang#92629 (Pick themes on settings page, not every page) - rust-lang#92640 (Fix ICEs related to `Deref<Target=[T; N]>` on newtypes) - rust-lang#92701 (Add some more attribute validation) - rust-lang#92803 (Hide mobile sidebar on some clicks) - rust-lang#92830 (Rustdoc style cleanups) - rust-lang#92866 ("Does exists" typos fix) - rust-lang#92870 (add `rustc_diagnostic_item` attribute to `AtomicBool` type) - rust-lang#92914 (htmldocck: Add support for `/text()` in ``@snapshot`)` - rust-lang#92923 (Abstract the pretty printer's ringbuffer to be infinitely sized) - rust-lang#92946 (Exclude llvm-libunwind from the self-contained set on s390x-musl targets) - rust-lang#92947 (rustdoc: Use `intersperse` in a `visit_path` function) - rust-lang#92997 (Add `~const` bound test for negative impls) - rust-lang#93004 (update codegen test for LLVM 14) - rust-lang#93016 (Stabilize vec_spare_capacity) Failed merges: - rust-lang#92924 (Delete pretty printer tracing) r? `@ghost` `@rustbot` modify labels: rollup
This hides the paintbrush icon on most pages by default, in preference for the settings on the settings page. When loading from a local file, and not in mobile view, continue to show the theme picker. That's because some browsers limit access to localStorage from file:/// URLs, so choosing a theme from settings.html doesn't take effect.
Fixes #84539
Part of #59840
r? @GuillaumeGomez
Demo: https://rustdoc.crud.net/jsha/theme-picker-local-only-2/std/io/trait.Read.html