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

Fix edge case in color-mode.js #39224

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

MewenLeHo
Copy link
Contributor

@MewenLeHo MewenLeHo commented Sep 22, 2023

Description

Our team found a potential edge case while working on our Bootstrap fork.

When using the current version of color-mode.js:

  const setTheme = theme => {
    if (theme === 'auto' && window.matchMedia('(prefers-color-scheme: dark)').matches) {
      document.documentElement.setAttribute('data-bs-theme', 'dark')
    } else {
      document.documentElement.setAttribute('data-bs-theme', theme)
    }
  }

The results will be:

color-mode on device color-mode on Bootstrap Result in HTML
* dark data-bs-theme="dark"
* light data-bs-theme="light"
dark auto data-bs-theme="dark"
light auto data-bs-theme="auto"

At this point you may have an edge case when your device is set to 'Light mode' and you use the 'Auto color mode' on the page because you will have to add more CSS to make something like:

@media (prefers-color-scheme: light) {
  [data-bs-theme="auto"] {
    // your code...
  }
}

Proposed patch:

  const setTheme = theme => {
- if (theme === 'auto' && window.matchMedia('(prefers-color-scheme: dark)').matches) {
-   document.documentElement.setAttribute('data-bs-theme', 'dark')
+ if (theme === 'auto') {
+   document.documentElement.setAttribute('data-bs-theme', (window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'))
  } else {

Then you will have:

color-mode on device color-mode on Bootstrap Result in HTML
* dark data-bs-theme="dark"
* light data-bs-theme="light"
dark auto data-bs-theme="dark"
light auto data-bs-theme="light"

And you don't need to add extra CSS everywhere.

Motivation & Context

Fix edge case when your device is set to 'Light mode' and you use the 'Auto color mode' on the page.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

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

Live previews

@MewenLeHo MewenLeHo requested a review from a team as a code owner September 22, 2023 12:26
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 🎉

I'm not in the JS team but it adds some consistency for color-mode. This looks safe to me to implement:

  • This doesn't interfer with any other mode than auto that we've been adding on Bootstrap.
  • It adds consistency over the data-bs-theme added on the HTML.
  • It defaults to the two commons color modes proposed by every browsers/OS.

@VibrantConcepts

This comment was marked as off-topic.

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @MewenLeHo

I can confirm the weird behavior in auto mode that switched between data-bs-theme="dark" (dark OS) and data-bs-theme="auto" (light OS). You fix allows to set the second use case to data-bs-theme="light" so that the behavior is consistent and allows to avoid handling "auto" in CSS.

@twbs/js-review, do you want to look at it too for a final review?

Note: when merged, we might want to:

  • Inform users in the release note if they copied & pasted this file to update it
  • Update our other repositories where a similar color-modes.js is used

@mdo mdo merged commit 73e1dcf into twbs:main Oct 31, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants