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: set default theme according to preferred color scheme #17311

Merged
merged 3 commits into from
Jul 1, 2022
Merged

fix: set default theme according to preferred color scheme #17311

merged 3 commits into from
Jul 1, 2022

Conversation

fabioatcorreia
Copy link
Contributor

Issue:
Currently, the theming documentation is not completely correct because "Unless you've set your preferred color scheme as dark" is not working.
ref #6088

What I did

Updated the theme under the defaultState to reuse the create function, which, when called without arguments, already returns the ThemeVars according to the preferred color scheme.

How to test

Open up a browser which supports the preferred color scheme preference, with any Storybook without a custom theme and, check that its theme is according to your preference.

@nx-cloud
Copy link

nx-cloud bot commented Jan 21, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 9340d52. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen
Copy link
Member

Hey @fabioatcorreia that's really sweet! Thank you for the contribution!!

@ndelangen
Copy link
Member

@shilman perhaps though not technically a BREAKING change, it might be perceived as disruptive one for users...
After upgrading their storybook might be dark all of a sudden, when they did not expect this.

WDYT, do we release this in a minor, or wait for a major?

@ndelangen
Copy link
Member

ndelangen commented Jan 24, 2022

We currently support this:

addons.setConfig({
  theme: themes.dark,
});

Users can currently do this:

addons.setConfig({
  theme: create(),
});

It would be great if users could configure:

addons.setConfig({
  theme: 'auto',
});

This would avoid the disruptive change but also allow users to easily opt-in to the automatic theme switching.

@fabioatcorreia
Copy link
Contributor Author

Sorry, but I couldn't understand if that last comment was meant for me or @shilman. @ndelangen?

@ndelangen
Copy link
Member

@fabioatcorreia I was expressing concern that users of storybook might find the change disruptive (storybook could turn to dark-mode by default) for a minor version bump.

And I was mentioning that users could already opt-in to the behaviour this PR sets.
I'm not sure how to proceed. I agree this PR should be merged, I just don't know when.

@literalpie
Copy link
Contributor

I think this is a great feature and I would love to have it, but I think adding dark mode could be breaking if any extensions are installed that do not support dark mode, or if someone is customizing the styles of SB.
If this does get merged, I would suggest clearly documenting a way to turn it off for anyone who has issues.

# Conflicts:
#	lib/api/src/modules/layout.ts
@ndelangen ndelangen merged commit 076f2f9 into storybookjs:next Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants