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

Change theme to dark if user has dark mode enabled using prefers-color-scheme media query #6088

Open
mohsen1 opened this issue Mar 14, 2019 · 17 comments

Comments

@mohsen1
Copy link

mohsen1 commented Mar 14, 2019

Chrome and Safari will soon support the prefers-color-scheme media query. Let's respect that since Storybook has a dark theme already

Are you able to assist bring the feature to reality?
yes

@shilman
Copy link
Member

shilman commented Mar 24, 2019

@mohsen1 Any interest in building this? If you need any help getting started, please join our discord!

@stale stale bot added the inactive label Apr 15, 2019
@shilman shilman removed this from the next milestone Apr 26, 2019
@stale stale bot removed the inactive label Apr 26, 2019
@stale stale bot added the inactive label May 17, 2019
@storybookjs storybookjs deleted a comment from stale bot May 18, 2019
@stale stale bot removed the inactive label May 18, 2019
@storybookjs storybookjs deleted a comment from stale bot May 18, 2019
@mathiasrando
Copy link
Contributor

It looks like this have stalled?

I'd like to help with this issue so I'll start working on an implementation.

@davidli3100
Copy link

@mathiasrando let me know if you aren't able to implement it. I've got a similar setup on my own projects I can adapt to storybook

@mathiasrando
Copy link
Contributor

mathiasrando commented Oct 2, 2019

Thanks @davidli3100!

I'm actually done implementing it, but have had a hard time finding a good way to test it due to matchMedia not being implemented in JSDOM and therefore not really being able to change the matches value in the mock during the course of a test in create.test.js(already has tests for picking light as the default theme).

The issue seems to be that it's required to import it before any functions using it(https://jestjs.io/docs/en/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom) and then overwriting it won't work.

I'll stick with adding tests for the new getPreferredColorScheme utility for now.

@shilman
Copy link
Member

shilman commented Oct 3, 2019

Whoopee!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-alpha.7 containing PR #8271 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Oct 3, 2019
@travi
Copy link

travi commented Jan 1, 2020

i upgraded to the latest pre-release for this ability, but it doesn't seem to work for me. My OS is set to dark mode, which works with other prefers-color-scheme situations, but i've also tried explicitly simulating through dev tools. did something regress?

i've pushed up a branch with my changes here if it helps provide context. npm start runs storybook for that project.

@shilman
Copy link
Member

shilman commented Jan 2, 2020

@mathiasrando can you help @travi out with this?

@shilman shilman reopened this Jan 2, 2020
@mathiasrando
Copy link
Contributor

That seems odd.

Things are a bit hectic at the moment, but I can take a look whenever I have an open slot.

What OS and browser are you using, @travi? It probably won't have an impact, but just to make sure 👍
And thanks for the branch.

@travi
Copy link

travi commented Jan 2, 2020

i'm using macOS Catalina and tested in both brave and chrome. i did clear local storage too, as mentioned in the referenced PR.

@mathiasrando
Copy link
Contributor

mathiasrando commented Jan 3, 2020

I gave it a quick look last night and to sum up:

  • Tests for the getPreferredColorScheme utility are passing
  • The getPreferredColorScheme utility is working correctly(both with emulation and actual OS color sheme) with manual testing
  • The applied theme never changes to dark

So I believe that the issue is related to the create method inside lib > theming > src > create.ts.
I think I'll be able to have a closer look today.

TLDR:
Some recent changes must have broken something related to applying the theme. I'll investigate further.

@mathiasrando
Copy link
Contributor

The getInitialState in the context provider always gets the light theme. It makes sense since the light theme is the default, but it also causes ensureTheme(state.theme) to always convert the light theme instead of the default themes[getPreferredColorScheme()].

I'm still unsure why it has stopped working since it doesn't seem like the use of create, convert and ensure have changed. My initial guess is that this commit has something to do with it, but I honestly don't know the project well enough to be sure.

I guess the solution would be to ensure that ThemeVars uses the getPreferredColorScheme-method when setting the initial state in lib > api > src > index.tsx, but unfortunately I currently don't have a good suggestion for resolving the issue.

@SachinCoder1
Copy link

Hi, is anyone working on it? i am new to open source, how do i get started?

@valar-dohaeris-29
Copy link

Is this still an issue? as I see a PR has been merged.

@ndelangen
Copy link
Member

It's been merged and works perfectly in the 7.0 alpha release!

@SuneRadich
Copy link

I am in the process of migrating from Storybook 6 to 7, and in that process I've just spent a couple of hours trying to figure out why all my storybooks suddenly went dark mode.
The documentation about theming mentions that the default theme is light, and only if you specifically set the theme to dark will it use it.

If the Storybook theme follows whatever mode is set on the OS/Browser level, that should also be mentioned in the documentation.

@ndelangen
Copy link
Member

@jonniebigodes that sounds like an documentation issue, would you be able to pick that up?

@jonniebigodes
Copy link
Contributor

@ndelangen Sure, thanks for letting me know of this. Appreciate it 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

10 participants