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

Simplify theme settings #98765

Closed

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 1, 2022

As the title states, the goal here is to simplify the theme settings by removing the setting of theme for dark/light system preference. Let me explain my logic behind this:

If you need to open the settings menu to change the used theme, then, why would you care about changing your preferred dark/light theme and not just update the theme directly?

As a secondary benefit, it simplifies the JS code handling themes.

You can test it here.

@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 Jul 1, 2022
@rust-highfive
Copy link
Collaborator

r? @notriddle

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2022
@GuillaumeGomez
Copy link
Member Author

r? @jsha

@rust-highfive rust-highfive assigned jsha and unassigned notriddle Jul 1, 2022
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Fixed eslint errors.

@Urgau
Copy link
Member

Urgau commented Jul 1, 2022

If you need to open the settings menu to change the used theme, then, why would you care about changing your preferred dark/light theme and not just update the theme directly?

You might have setup your system so that the system theme (and so your browser theme) follows the day/night cycle of your location and by that means to might want to set day=light and night=ayu. And if I understand correctly this PR this wouldn't be possible anymore, right ?

@GuillaumeGomez
Copy link
Member Author

You could still follow the day cycle but not pick which dark/light theme is picked indeed.

@Urgau
Copy link
Member

Urgau commented Jul 1, 2022

You could still follow the day cycle but not pick which dark/light theme is picked indeed.

🤔 That not great. I could see people wanting this setup (there probably are people currently having it). Would be great to at least have a alternative if this PR lands.

@GuillaumeGomez
Copy link
Member Author

I wonder how many people use this setup. Another thing I was planning to do was to switch the default dark theme to ayu. Might be interesting to see how many people have a day/light setting with ayu/dark as default dark theme.

@apiraino
Copy link
Contributor

apiraino commented Jul 2, 2022

You might have setup your system so that the system theme (and so your browser theme) follows the day/night cycle of your location

I was also confused by this at the beginning. Websites provide a dual setting, tipically the 🌙 / 🌞 icon with no explicit option for a "system default".

On MDN, the documentation for the prefer-color-scheme says:

  • light: means "use light theme" or no active user preference
  • dark means "use dark theme"

So I think that a "system setting" will be automatically used when flipping the switch to light. Therefore no explicit "use system setting" should be needed.

Makes sense?

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jul 2, 2022

I'm not a big fan of implicit. If we want to allow users to switch back to "system preference", we should maybe just add a new entry in the theme list.

@steffahn
Copy link
Member

steffahn commented Jul 2, 2022

I like the idea of having "system preference" simply as a 4th option.

The most minimalistic additional configuration option to serve most use cases (in my opinion), given that we have one light theme and two dark themes, is to simply add a second option to choose the preferred dark theme. In order to make this choice minimally confusing, I would suggest that it only becomes visible if:

  • the "system preference" option is chosen, and
  • the system settings currently are set to dark-themed

this way, every change of preference will always become immediately visible, there’s no hypothetical/dead options that would only take effect under certain conditions (w.r.t. system preferences) that you might never encounter, and there’s no weird options of calling dark themes “the preferred light theme” or vice-versa.

Here’s some mock-up screenshots of all possible states:

ayu

Screenshot_20220702_165638

dark

Screenshot_20220702_165649

light

Screenshot_20220702_165709

system preference (system theme is currently light-themed), preferred dark theme is saved but not displayed

Screenshot_20220702_165425

system preference (system theme is currently dark-themed), preferred dark theme is ayu

Screenshot_20220702_165310

system preference (system theme is currently dark-themed), preferred dark theme is dark

Screenshot_20220702_165133

@apiraino
Copy link
Contributor

apiraino commented Jul 2, 2022

(not an expert, just my .2 cents)

Although from a UX point of view expliciting the option "use system theme" would be better, why can't I think of websites allowing such an option? 🤷 This is why I thought that a "use system default" was redundant and my understanding of the documentation for the CSS directive seems to confirm that.

The second menu option for the theme selection:
grafik

is more or less that we have now on rustdoc stable and imho it's a bit confusing, so in this regard I'm with @GuillaumeGomez in trying to reduce the number of actionable knobs.

@notriddle
Copy link
Contributor

notriddle commented Jul 2, 2022

Alternative mockup, based roughly on how GitHub's screen works, but with the silly option to set your dark theme to light removed.

With "Use system theme" turned on

image

With "Use system theme" turned off

image

The reason I don't like the four-option mockup is that it precludes adding a second light theme. I would like to eventually clone the Rust theme from mdBook.

For the record, here's what GitHub's screen looks like

With mode set to "Sync with system"

image

With mode set to "Single theme"

image

@steffahn
Copy link
Member

steffahn commented Jul 2, 2022

The reason I don't like the four-option mockup is that it precludes adding a second light theme.

Within the context of what I presented above, a 2-light-themes + 2-dark-themes situation would be modeled without any problem; the idea that the “preferred dark theme” choice is only visible while “system preference” is active and the system preference is currently set to dark-themed (in order to avoid confusion from options that don’t have any immediate effect) translates into the same idea for “preferred light theme” option visibility, so you only see one-at-a-time. I.e. it looks like this

system preference (system theme is currently light-themed), preferred light theme is light, preferred dark theme is saved but not displayed

Screenshot_20220702_210815

system preference (system theme is currently light-themed), preferred light theme is rust, preferred dark theme is saved but not displayed

Screenshot_20220702_210739
(I only changed the background color, this is not a real theme)

system preference (system theme is currently dark-themed), preferred dark theme is ayu, preferred light theme is saved but not displayed

Screenshot_20220702_210857

system preference (system theme is currently dark-themed), preferred dark theme is dark, preferred light theme is saved but not displayed

Screenshot_20220702_210911

some theme selected unconditionally (e.g. dark theme)

Screenshot_20220702_210932

To be clear, the choice of label for “system preference” I don’t exactly care about as long as it’s some easy-to-understand label.


One important consequence of this approach is: Whatever you click on is always what you get, immediately; and you can always choose any theme.

  • You click on any theme in the top row and immediately get it. Single step, trivial discoverability. Especially considering that users typically start out on “system preference” by default, it’s important that all themes are visibly somewhere, otherwise you might not even know about dark themes existing in the first place despite looking into the options.
  • If you click on “system preference”, you will get a light theme with light system preference, and a dark theme with dark system preference, because the option to select light “preferred dark theme”s or dark “preferred light theme”s is gone
  • There is e.g. two ways of choosing a dark theme while in dark-themed system preference mode, but both get you the right theme immediately, the difference only matters if your system theme changes, and you can easily fix the problem after the fact.
  • Admitted, configuring a setup for using the system theme with custom choices for both dark and light theme ahead of time is no longer possible, but it should rarely be really necessary anyways. Someone who uses changing system themes on a day-night-cycle will have to choose their preferred day theme once while it’s currently daytime and they will have to choose their preferred night theme once while it’s currently nighttime, but the benefits you get for this limitation is a more intuitive user experience, because at least it’s always: what you click on is what you see and what you get.

By contrast (to the last point about getting what you click on), assuming I’m not misinterpreting your mock-up, what your suggesting is: that the “preferred dark theme” option is visible while “use system theme” is on, and the system theme is light-themed, (which means by the way, that there’s a selection for dark themes that can seem kind-of broken to users, because it doesn’t have any visible immediate effects), and that the real option for selecting a dark theme isn't visible while “use system theme” is on. Essentially users would have to

  • ignore the visible button labeled “dark” (in the “preferred dark theme” section) … but hey, I do prefer dark themes!?
  • press the button labeled “use system theme” in order to not use the system theme (I know it’s an on/off switch, but that might not be obvious), then (it’s a 2-step-process!) click the newly appearing option to use a button labeled “dark” that looks almost identical to the button labeled “dark” that you previously had to ignore.

@notriddle
Copy link
Contributor

notriddle commented Jul 2, 2022

Yeah, the mock-ups from #98765 (comment) are better than the ones I put together.

@steffahn
Copy link
Member

steffahn commented Jul 2, 2022

To be clear, the choice of label for “system preference” I don’t exactly care about as long as it’s some easy-to-understand label.

For example, the label could also say “sync with system” (like the GitHub preferences). Furthermore the title of the “preferred … theme” sections could also more clearly relate to this setting by saying something like “Dark theme for “sync with system””.
Screenshot_20220702_211323

@GuillaumeGomez
Copy link
Member Author

I think it's a good idea to show only the linked theme in case "sync with system" is used. I'll update the PR.

@GuillaumeGomez
Copy link
Member Author

I updated the PR and the online version. So I took the idea of having the "preferred settings" when selecting "system preference". It's much more intuitive this way in my opinion. So what do you think?

@notriddle
Copy link
Contributor

I think it's a good idea to show only the linked theme in case "sync with system" is used. I'll update the PR.

When I have sync turned on, I’m seeing both themes, not just the active one.

Screenshot 2022-07-12 at 7 42 47 AM

@GuillaumeGomez
Copy link
Member Author

Yup. It's as expected.

@GuillaumeGomez
Copy link
Member Author

cc @steffahn and @apiraino :)

@GuillaumeGomez
Copy link
Member Author

I fixed the theme switching bug. I now also get the "use-system-theme" setting when there is no "rustdoc-theme2" set. I updated the online demo too.

@steffahn
Copy link
Member

steffahn commented Jul 28, 2022

Still behaves weird in common cases. If I navigate to https://rustdoc.crud.net/imperio/improve_rustdoc_search_results_page_crates_selection/foo/index.html in an incognito tab, then switch to https://rustdoc.crud.net/imperio/simplify-theme-settings/foo/index.html then I get into a “system preference” state without the further “Preferred … theme” options visible.
Screenshot_20220728_181538

(The only value in local storage at that point is “rustdoc-theme: dark”.)


I’d have to read and understand more of all that js code to get an idea as to why it’s behaving the way it is.


I might also try to test out some time in the next few days how straightforward a solution to “just keep using the existing storage values” might be. Perhaps, adding a “legacyTranslation” function for reading from and writing to local storage that translates “theme2” from and to “theme”+“use-system-theme” accordingly.

@GuillaumeGomez
Copy link
Member Author

The settings were not checking the currently set values. Fixed it and added a test for it. Thanks a lot for checking this!

@GuillaumeGomez
Copy link
Member Author

Did you have time to take another look @steffahn ?

@bors
Copy link
Contributor

bors commented Sep 4, 2022

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 5, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@GuillaumeGomez
Copy link
Member Author

Fixed conflict.

@steffahn
Copy link
Member

steffahn commented Sep 5, 2022

Sorry I haven’t been able to look back into this yet. I likely won’t have much time for the next 3-4 weeks either, though I’m probably able to do a round of testing again, not necessarily looking into the code though.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Sep 5, 2022

It's fine, don't worry. It's not really in a hurry. Just wanted to be sure what was worrying you was fixed.

@bors
Copy link
Contributor

bors commented Sep 28, 2022

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 17, 2022
…heme-choice, r=notriddle

Simplify settings theme choice

I removed the storage changes from rust-lang#98765 and only kept the UI changes.

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

Discussion about this still in progress [on zulip](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Last.20part.20of.20settings.20simplification).

r? `@notriddle`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 17, 2022
…heme-choice, r=notriddle

Simplify settings theme choice

I removed the storage changes from rust-lang#98765 and only kept the UI changes.

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

Discussion about this still in progress [on zulip](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Last.20part.20of.20settings.20simplification).

r? ``@notriddle``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 17, 2022
…heme-choice, r=notriddle

Simplify settings theme choice

I removed the storage changes from rust-lang#98765 and only kept the UI changes.

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

Discussion about this still in progress [on zulip](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Last.20part.20of.20settings.20simplification).

r? ```@notriddle```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 17, 2022
…heme-choice, r=notriddle

Simplify settings theme choice

I removed the storage changes from rust-lang#98765 and only kept the UI changes.

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

Discussion about this still in progress [on zulip](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Last.20part.20of.20settings.20simplification).

r? ````@notriddle````
@GuillaumeGomez
Copy link
Member Author

The UI change was merged in #104366 so closing this one.

@GuillaumeGomez GuillaumeGomez deleted the simplify-theme-settings branch November 21, 2022 09:06
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 A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants