-
-
Notifications
You must be signed in to change notification settings - Fork 78.8k
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
Update color-modes.js #38626
Update color-modes.js #38626
Conversation
Fix IF statement in the prefer-color-scheme change listener always evaluating to `true` and changing the theme to "dark" even when "light" is set as the preferred theme. | `||` | `x !== "light"` | `x !== "dark"` | Result | |--|:--:|:--:|:--:| | `x = "light"` | ○ | ● | ● | | `x = "dark"` | ● | ○ | ● | | `x = "auto"` | ● | ● | ● | | `x = "bogus"` | ● | ● | ● | <hr> | `&&` | `x !== "light"` | `x !== "dark"` | Result | |--|:--:|:--:|:--:| | `x = "light"` | ○ | ● | ○ | | `x = "dark"` | ● | ○ | ○ | | `x = "auto"` | ● | ● | ● | | `x = "bogus"` | ● | ● | ● |
Thanks for this PR @Ionaru While testing your modification (which seems to be an improvement), I'm wondering if there's not also another bug.
Example of use case that doesn't work:
I'm wondering if we shouldn't do something like that (or transform diff --git a/site/static/docs/5.3/assets/js/color-modes.js b/site/static/docs/5.3/assets/js/color-modes.js
index 4528ba36b..c8bdf7d6f 100644
--- a/site/static/docs/5.3/assets/js/color-modes.js
+++ b/site/static/docs/5.3/assets/js/color-modes.js
@@ -7,11 +7,9 @@
(() => {
'use strict'
- const storedTheme = localStorage.getItem('theme')
-
const getPreferredTheme = () => {
- if (storedTheme) {
- return storedTheme
+ if (localStorage.getItem('theme')) {
+ return localStorage.getItem('theme')
}
return window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'
@@ -56,7 +54,7 @@
}
window.matchMedia('(prefers-color-scheme: dark)').addEventListener('change', () => {
- if (storedTheme !== 'light' || storedTheme !== 'dark') {
+ if (localStorage.getItem('theme') !== 'light' && localStorage.getItem('theme') !== 'dark') {
setTheme(getPreferredTheme())
}
}) /CC @mdo |
@julien-deramond You're completely right. I also detected that it wasn't perfect but couldn't nail it down, with your steps I was able to reproduce the issue. While I'm personally not a fan of repeating I've pushed an update with your suggestions implemented. |
Yep definitely, repeating Regarding your other comment, I completely agree. This snippet could stay as-is and not be such an issue for a first version because not that blocking and it'll rarely occur. Waiting for other thoughts from the rest of the core team. |
* Update color-modes.js Fix IF statement in the prefer-color-scheme change listener always evaluating to `true` and changing the theme to "dark" even when "light" is set as the preferred theme. | `||` | `x !== "light"` | `x !== "dark"` | Result | |--|:--:|:--:|:--:| | `x = "light"` | ○ | ● | ● | | `x = "dark"` | ● | ○ | ● | | `x = "auto"` | ● | ● | ● | | `x = "bogus"` | ● | ● | ● | <hr> | `&&` | `x !== "light"` | `x !== "dark"` | Result | |--|:--:|:--:|:--:| | `x = "light"` | ○ | ● | ○ | | `x = "dark"` | ● | ○ | ○ | | `x = "auto"` | ● | ● | ● | | `x = "bogus"` | ● | ● | ● | * Implement re-read of stored theme
Description
Fix the IF statement in the prefer-color-scheme change listener always evaluating to
true
and changing the theme even when "light" or "dark" is set as the preferred theme.Motivation & Context
In some situations, toggling the
prefer-color-scheme
using the devtools causes the theme to switch and display incorrectly in the UI.I believe this is partially caused by this non-functional IF statement:
Because
storedTheme
can never be both "light" and "dark" at the same time.||
x !== "light"
x !== "dark"
x = "light"
x = "dark"
x = "auto"
x = "bogus"
&&
x !== "light"
x !== "dark"
x = "light"
x = "dark"
x = "auto"
x = "bogus"
This behaviour can be seen on https://getbootstrap.com/docs/5.3 by playing with the light/dark toggle in combination with the devtools rendering emulations.
Type of changes
Checklist
npm run lint
)Live previews
Related issues