Theme switcher revised #2023
Theme switcher revised #2023
Conversation
…hods for theme management
| #applyStoredTheme() { | ||
| const storedTheme = this.#storedTheme | ||
|
|
||
| if (storedTheme === "light") { |
There was a problem hiding this comment.
could invert to make more concise:
if (storedTheme === "auto") {
document.documentElement.removeAttribute("data-theme")
} else {
document.documentElement.setAttribute("data-theme", storedTheme)
} But maybe we can even do just"
document.documentElement.setAttribute("data-theme", this.#storedTheme)auto won't have any style handling, so that is fine, right?
There was a problem hiding this comment.
In CSS, we currently check for the presence of data-theme. If we're saving data-theme="auto" in the JS controller, we'll need to update the CSS to html:not([data-theme="light"])
@media (prefers-color-scheme: dark) {
html:not([data-theme]) & {
box-shadow: 0 0 0 1px var(--color-ink-lighter);
}
}
There was a problem hiding this comment.
I pushed a couple small improvements to reduce the repetition of inline SVGs, but other than that this is looking nice! 👍 We can revisit in a few months when it looks like light-dark() has better support. We'll be able to remove a couple hundred lines of CSS, I'd wager.
jorgemanrubia
left a comment
jorgemanrubia
left a comment
There was a problem hiding this comment.
couple of minor comments but looks great 👏
|
Closes #1935 |
The implementation in #1935 is solid but needs some design polish: