Skip to content

Revert to binary light/dark mode toggle, remove osd-theme-system option - #591

Open
oswwwaldo wants to merge 1 commit into
opensourcedesign:mainfrom
oswwwaldo:main
Open

Revert to binary light/dark mode toggle, remove osd-theme-system option#591
oswwwaldo wants to merge 1 commit into
opensourcedesign:mainfrom
oswwwaldo:main

Conversation

@oswwwaldo

Copy link
Copy Markdown
Contributor

Closes: Color mode has invisible tri-state button; use two state or dropdown
#588

I think I covered everything, I am still relatively new to this codebase (and technologies) but I've tested it on my end and it works perfectly. Would like a review and push to main!

Remove osd-theme-system button, clean up dead CSS, add overwrite legacy "system" localStorage for returning users. Move meta tags to top of head (closes hint-meta-charset-utf-8). Closes Color mode has invisible tri-state button; use two state or dropdown
Fixes opensourcedesign#588
@github-actions

Copy link
Copy Markdown
Contributor

PR site preview

Preview this pull request: https://opensourcedesign.net/pr-preview/pr-591/

@jdittrich

Copy link
Copy Markdown
Member

Works well for me – not an expert on light/dark-mode design (I was pointing out a good-old-fashioned usability problem), so happy about others who check this.

@SaptakS

SaptakS commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

I have not fully reviewed this PR, but the reference logic that I have started using these days for color mode switch is https://lea.verou.me/blog/2026/dark-mode-toggles/. And, that does use a binary system, but also respects the color mode of the system (TL;DR of that article is basically the binary switch shows the color mode of the system, and then you can switch to change it)

@SaptakS SaptakS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly okay for me now. What I don't like is if system is in dark mode, I load the website, it sets the website to dark mode which is great. But since that value is now in localStorage, if I switch system to light mode, it still stays in dark mode till I manually switch it. So the system color mode detection only happens the first time the website is ever loaded.

If possible, I would like an implementation something like this:

  • on load, the localstorage value is not set if it's not already set and we fallback to system color scheme, only toggle the root classList based on the preference.
  • on load, the localstorage value is unset if the system color mode and the current value in localstorage are the same (since basically its a fallback to the system color)
  • if press the button, then always set localStorage.

This is what makes the most sense to me to detect the system as well as user's preference with a 2 way toggle. @AnXh3L0 @jdittrich thoughts?

If people don't agree, this PR is a good first step, and is good to be merged.

@oswwwaldo

Copy link
Copy Markdown
Contributor Author

I can see about implementing these changes, I didn't think of these edge cases so it's a good catch. Will bring a set of commits to this PR soon if no other changes are requested, also thank you for the read @SaptakS ! It was a surprisingly fun and informative blog post... and I didn't realize just how complicated these things can get 😅

@jdittrich

Copy link
Copy Markdown
Member

This is what makes the most sense to me to detect the system as well as user's preference with a 2 way toggle

Makes sense to me. So the algorithm would be something like:

FUNCTION setDisplayMode:
    IF THERE IS NO localStorageValue
      use system default
    ELSE THERE IS localStorageValue
      IF localStorageValue IS systemDefaultValue
          remove storage value
          use system default
      ELSE  localStorageValue IS NOT systemDefaultValue
          use storage value

ON PAGE LOAD:
    run setDisplayMode

ON TOGGLE:
    set localStorageValue to ToggleValue
    run setDisplayMode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants