Update color-modes.js

[Ionaru]

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:

storedTheme !== 'light' || storedTheme !== 'dark'

Because storedTheme can never be both "light" and "dark" at the same time.

Current: ||	x !== "light"	x !== "dark"	Result
x = "light"	○	●	●
x = "dark"	●	○	●
x = "auto"	●	●	●
x = "bogus"	●	●	●

New: &&	x !== "light"	x !== "dark"	Result
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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Refactoring (non-breaking change)
• Breaking change (fix or feature that would change existing functionality)

Checklist

• I have read the contributing guidelines
• My code follows the code style of the project (using npm run lint)
• My change introduces changes to the documentation
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed

Live previews

• https://deploy-preview-38626--twbs-bootstrap.netlify.app/

Related issues

[julien-deramond]

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.

storedTheme is only set from the local storage when the script/page is loaded, and is never changed again. So there are use cases where one can play with the OS light/dark/auto appearance, and the color displayed in the documentation is the opposite.

Example of use case that doesn't work:

1. Choose OS appearance as dark, load the website and choose "Auto", reload the page
2. Change OS appearance to light → website is light 🟢
3. Change OS appearance to dark → website is dark 🟢
4. In the dropdown, choose "Dark" → website stays in dark 🟢
5. Change OS appearance to light → website is light 🔴

I'm wondering if we shouldn't do something like that (or transform storedTheme as a function that gets the value from the local storage):

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

[Ionaru]

@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 localStorage.getItem('theme'), I also realise this snippet is just an example for developers to implement their own color-scheme-switchers. However it will probably be copy-pasted a lot. 😅
So I think it's best to wrap that in a function.

I've pushed an update with your suggestions implemented.

[julien-deramond]

While I'm personally not a fan of repeating localStorage.getItem('theme'), I also realize this snippet is just an example for developers to implement their own color-scheme-switchers. However, it will probably be copy-pasted a lot. 😅
So I think it's best to wrap that in a function.

Yep definitely, repeating localStorage.getItem('theme') wasn't elegant at all 😇 . It was mostly when I tested the use cases just to be 100% sure of the value. The function version is way better!

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.
But the important part is that, yeah, it'll be copy-pasted and probably never updated later by the majority. This is why I'd like to ship this fix for the v5.3.0 if that's still possible.

Waiting for other thoughts from the rest of the core team.