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

Theme toggle UI has no ability to return to default “follow system theme” #2539

Closed
flying-sheep opened this issue Apr 19, 2022 · 17 comments · Fixed by #2552
Closed

Theme toggle UI has no ability to return to default “follow system theme” #2539

flying-sheep opened this issue Apr 19, 2022 · 17 comments · Fixed by #2552
Labels
infra Core infrastructure for building and rendering PEPs

Comments

@flying-sheep
Copy link
Contributor

flying-sheep commented Apr 19, 2022

By default the page behaves perfectly: Dark when the system prefers dark mode, light if it prefers a light mode.

But once you click the theme toggle, you irrevocably break this behavior (until the user figures out how to clear cookies): The site is now stuck and no longer follows the system theme.

The button UI should work exactly like in the Furo theme be able to toggle between “auto”, “dark” and “light” instead.

@CAM-Gerlach CAM-Gerlach added the infra Core infrastructure for building and rendering PEPs label Apr 19, 2022
@CAM-Gerlach
Copy link
Member

I agree that makes more sense, though we'd name to change a fair bit of the logic and add icons for the other two states to distinguish them, and it also reverts part of the change introduced in #2494 in response to #2487 .

@wookie184 @hugovk what do you think about this? Both of you presumably have better JS-fu than me in this department...

@flying-sheep flying-sheep changed the title Theme toggle button should be tri-state Theme toggle button destroys ability to return to default “follow system theme” Apr 20, 2022
@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 20, 2022

I thought about this a bit, and changed the request to no longer be about a specific UX choice.

This is now a bug report, not a feature request. The UI lacks the ability to return to the default state, this needs to be fixed.

I think there’s two solutions that make sense:

  1. there’s no UI, just CSS automatically reacting to the system theme
  2. there’s UI that handles all three possible states: “auto”, “dark” and “light”

Discussion makes sense about which of the two, and if the second one, how UI will look (tri state button, tri state toggle switch, two state button and “revert” button, …)

@flying-sheep flying-sheep changed the title Theme toggle button destroys ability to return to default “follow system theme” Theme toggle UI has no ability to return to default “follow system theme” Apr 20, 2022
@hugovk
Copy link
Member

hugovk commented Apr 20, 2022

Solution 1 (system only) isn't great for accessibility: some people may need to change to another mode than their system:

https://www.boia.org/blog/dark-mode-can-improve-text-readability-but-not-for-everyone

Solution 2 (tri-state toggle): I couldn't really find anything from an accessibility point-of-view, but I think this is better than solution 1 and the status quo (two-state toggle).

As an example, the Furo theme has a tri-state toggle, and we're already using one of the three icons also used there. We could use the same logic/sequence too, it's quite a popular theme and familiar behaviour would be a plus.

@AA-Turner
Copy link
Member

I agree the status-quo is not perfect, though my concern about the three-state model is that the third state can be hard to identify. Dark vs normal is fine, but if you start with the system requesting the default colour scheme, the first click goes to dark, the second click goes back to light, and the third click stays light (but back to system preference).

A

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 20, 2022

The status quo doesn’t have a cosmetic flaw, it has a bug. I agree that it’s hard to figure out satisfactory UX, that’s why UX designers exist. My suggested solution was only initially ”Furo’s three state toggle or bust”, then I changed it to “some UI that allows to switch between the 3 states that exist”.

It doesn’t need to be a single toggle. It could also be e.g.

  1. A dropdown / radio buttons. I agree, that wouldn’t be cute and would require 2 clicks instead of 1, but as said: We have a bug here, not a question of taste. Something functional is preferable to something pretty, but broken.

  2. Like firefox devtools, a “force light mode” and a “force dark mode” button. They behave like radio buttons in that only one can be switched on at a time, but you can also turn them both off to go back to the system theme.

  3. a toggle button and a revert button with these states

    • 🌓

    • 🌙

    • 🌞

      flowchart
      Auto("Auto theme dependent on system theme:\nToggle button is 🌓, ⎌ button hidden")
      Dark("Dark theme chosen:\nToggle button is 🌙, ⎌ button shown")
      Light("Light theme chosen:\nToggle button is 🌞, ⎌ button shown")
      X["Toggle between dark and light"]
      
      Auto -->|"🌓 clicked when\nsystem theme is light"| Dark
      Auto -->|"🌓 clicked when\nsystem theme is dark"| Light
      Dark & Light -->|"⎌ clicked"| Auto
      Dark -->|"🌙 clicked"| X
      Light -->|"🌞 clicked"| X
      
      Loading

We could use the same logic/sequence too, it's quite a popular theme and familiar behaviour would be a plus.

I agree, that’s a good argument to go with “just do what furo does”. Consistency is a nice thing to do to people. Curious people could also get a tooltip like “System theme. Click to switch to light theme”

@Rosuav
Copy link
Contributor

Rosuav commented Apr 20, 2022

Two clicks instead of one isn't that bad when we're talking about theme changing. It's not like people will need to be constantly flipping the theme back and forth. I'd be inclined to the dropdown.

@hugovk
Copy link
Member

hugovk commented Apr 20, 2022

The Furo logic:

  if (prefersDark) {
    // Auto (dark) -> Light -> Dark
    if (currentTheme === "auto") {
      setTheme("light");
    } else if (currentTheme == "light") {
      setTheme("dark");
    } else {
      setTheme("auto");
    }
  } else {
    // Auto (light) -> Dark -> Light
    if (currentTheme === "auto") {
      setTheme("dark");
    } else if (currentTheme == "dark") {
      setTheme("light");
    } else {
      setTheme("auto");
    }

When system prefers dark:

flowchart LR
Auto("Auto (dark) 🌓")
Light("Light 🔆")
Dark("Dark 🌙")

Auto-->|"click 🌓" | Light-->|"click 🔆"| Dark-->|"click 🌙"| Auto
Loading

Otherwise:

flowchart LR
Auto("Auto (light) 🌓")
Light("Light 🔆")
Dark("Dark 🌙")

Auto-->|"click 🌓" | Dark-->|"click 🌙"| Light-->|"click 🔆"| Auto
Loading

https://pradyunsg.me/furo/

@CAM-Gerlach
Copy link
Member

For the reasons others have mentioned, of the options presented the UI Furo uses seems by far the most intuitive and elegant and the least klunky and intrusive.

I assume the "solution" of completely removing the functionality was not being presented seriously, given its hard to see how it wouldn't be less practically useful than the current behavior (especially for users on systems, like mine, that don't even natively support OS-level theming—at least as far as I know).

But, putting practical utility aside for a moment, if we indeed accept the current behavior to be a bug due to offering an initial state and not allowing users to return to it (as opposed to simply treating it as an initial default), then purely in terms of not introducing said bug, we must acknowledge an equally-valid third approach—not auto-detecting the state from OS-level theming, and just defaulting to one or the other with a toggle between.

Sure, in practical terms that's a regression, but so is completely removing the ability to toggle such, and at least with the former it only takes at most one click for users to set the state they prefer, whereas if the desired appearance for the PEPs doesn't match what their OS is set to, or their OS doesn't actually have theming support, or their browser doesn't support it, etc,, then they are simply out of luck.

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 21, 2022

Ah, if you use a OS with no global dark/light mode, you’ll need to think about how it behaves for someone who does. On OSX, you can e.g. set up your system to switch between light and dark mode depending on time of day. A stylesheet that follows the system theme supports this. Manually clicking a button for every page twice a day doesn’t.

That’s why that option didn’t come up: There’s a perfectly functional solution with no potential for confusion (a dropdown). All other solutions should be measured by that standard. “Slightly prettier/cuter but less/not functional” isn’t a good trade.

For the record: I also think the Furo UI is perfectly fine. The worst that can happen is that people click, see no change, click again, and have what they want. Good enough for me.

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 22, 2022

pydata-sphinx-theme also has a furo-style three state button: https://pydata-sphinx-theme.readthedocs.io/en/latest

@hugovk
Copy link
Member

hugovk commented Apr 22, 2022

Added recently in pydata/pydata-sphinx-theme#540, the logic looks pretty similar to Furo.

@CAM-Gerlach
Copy link
Member

A stylesheet that follows the system theme supports this. Manually clicking a button for every page twice a day doesn’t. That’s why that option didn’t come up: There’s a perfectly functional solution with no potential for confusion (a dropdown). All other solutions should be measured by that standard. “Slightly prettier/cuter but less/not functional” isn’t a good trade.

I understand that, but what I didn't get was why the option to completely remove the manual selection entirely and prevent users from switching themes at all, except through their OS if both it and their browser support such, did come up, which is by the same taken also an equivalently bad trade, as it also is not only inconvenient but completely unusable for a substantial fraction of use cases.

In any case, we both agree that neither is ideal, of course, and the Furo UX/UI neatly solves this while providing the behavior you request.

The worst that can happen is that people click, see no change, click again, and have what they want. Good enough for me.

And in fact, Furo's logic that @hugovk mentions above ensures that the first time they click (from automatic mode) they will get a change; it is only on their third click changing it back to automatic mode will it not (which doesn't seem to be a problem to me).

@flying-sheep
Copy link
Contributor Author

we both agree that neither is ideal, of course, and the Furo UX/UI neatly solves this while providing the behavior you request.

exactly! so would a PR to introduce that change be welcome?

@AA-Turner
Copy link
Member

would a PR to introduce that change be welcome?

Yes, thank you.

A

@pradyunsg
Copy link
Member

pradyunsg commented Apr 23, 2022

The Furo logic:

I am glad that Furo is being used as a reference here. ^.^

I don't have any opinions to add to the choice between 3-state-slider / dropdown / 3-state-button. One thing I do want to say though: I think any "pin this theme" logic should be supported by a first-render-blocking <script> tag to set the theme. Not seeing a frame-or-two of light mode when my system is set to light mode and my preference is pinned to dark is A Good Thing™. :)

Here's where Furo does that: https://github.com/pradyunsg/furo/blob/6b6610fd1717756d210e7f32d34f935fffa6939a/src/furo/theme/furo/base.html#L86.

Edit to add more context: Furo also uses localStorage to remember what the user preference is, across pages.

@AA-Turner
Copy link
Member

Moving colour_scheme.js to be loaded at the start of <body> would achieve the same effect of blocking rendering until the colour-theme support is loaded, I believe (move the below line to L21)

<script src="{{ pathto('_static/colour_scheme.js', resource=True) }}"></script>

A

@pradyunsg
Copy link
Member

Moving colour_scheme.js to be loaded at the start of <body> would achieve the same effect of blocking rendering until the colour-theme support is loaded, I believe (move the below line to L21)

I believe it would work as well.

I inlined the relevant logic in Furo, to avoid the additional network request -- https://developers.google.com/speed/docs/insights/BlockingJS has some notes on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Core infrastructure for building and rendering PEPs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants