Theming: Set themes.normal according to user preference and export getPreferredColorScheme#28721
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
valentinpalkovic
added
theming
addon: docs
bug
ci:normal
|
View your CI Pipeline Execution ↗ for commit 99d61a0
☁️ Nx Cloud last updated this comment at |
ndelangen
left a comment
ndelangen
left a comment
There was a problem hiding this comment.
@cdedreuille are you ok with this?
|
I just found this, when I was setting a import { addons } from '@storybook/manager-api'
import { themes } from '@storybook/theming'
addons.setConfig({
theme: {
...themes.normal,
brandImage: 'path/to/my/logo.svg',
},
})and the dark mode following the system preferences would not work anymore. The development of the MR seems to be finished but it was not merged yet, is there anything which needs to be done still? @ndelangen Looks like @cdedreuille didn’t find the time to review this, is there anyone else who could bring this forward? |
valentinpalkovic
force-pushed
the
next-8.5
branch
from
b982c44 to
9f371ba
Compare
|
I'm not entirely certain what side effects might occur here, but overall I think this is a good idea. I do need another engineer on the team to take a look at it. @Sidnioulz Are you checking this out? I am also curious about the bug reported by @thomasaull |
📝 WalkthroughWalkthroughAdds getPreferredColorScheme to global and theming exports. Updates theme creation to compute themes.normal from the preferred color scheme. Introduces internal Themes and themesBase types/constants. Extends tests to mock preference and verify normal resolves to light or dark accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Consumer
participant Theming as theming/create
participant Utils as theming/utils
App->>Theming: import { themes }
activate Theming
Theming->>Utils: getPreferredColorScheme()
activate Utils
Utils-->>Theming: "light" | "dark"
deactivate Utils
Theming->>Theming: build themesBase {light, dark}
Theming->>Theming: set normal = themesBase[preferred]
Theming-->>App: export themes {light, dark, normal}
deactivate Theming
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/theming/tests/create.test.js (1)
6-8: Align mock setup with coding guidelines.The mock should include the
spy: trueoption and avoid inline implementations.As per coding guidelines:
- "Use vi.mock() with the spy: true option for all package and file mocks"
- "Mock implementations should be placed in beforeEach blocks"
Apply this diff:
-vi.mock('./../utils', () => ({ - getPreferredColorScheme: vi.fn().mockReturnValue('light'), -})); +vi.mock('./../utils', { spy: true });Then add a beforeEach block in the 'themes' describe block to set the default behavior:
describe('themes', () => { beforeEach(() => { vi.resetModules(); + vi.mocked(getPreferredColorScheme).mockReturnValue('light'); });code/core/src/theming/create.ts (1)
18-18: Confirm SSR fallback and consider reactive updates
getPreferredColorSchemefalls back to'light'whenwindow.matchMediais unavailable (e.g. SSR), so server safety is covered. Since it’s evaluated once at module load, system preference changes at runtime won’t updatethemes.normal—consider re-evaluating on mount or subscribing toprefers-color-schemechange events for dynamic updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/core/src/manager/globals/exports.ts(3 hunks)code/core/src/theming/create.ts(1 hunks)code/core/src/theming/index.ts(1 hunks)code/core/src/theming/tests/create.test.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/manager/globals/exports.tscode/core/src/theming/create.tscode/core/src/theming/tests/create.test.jscode/core/src/theming/index.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/manager/globals/exports.tscode/core/src/theming/create.tscode/core/src/theming/tests/create.test.jscode/core/src/theming/index.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/manager/globals/exports.tscode/core/src/theming/create.tscode/core/src/theming/tests/create.test.jscode/core/src/theming/index.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/globals/exports.tscode/core/src/theming/create.tscode/core/src/theming/tests/create.test.jscode/core/src/theming/index.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/core/src/theming/tests/create.test.js
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/core/src/theming/tests/create.test.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/theming/index.ts (1)
43-43: LGTM! Clean addition to the public API.The export of
getPreferredColorSchemealigns with the PR objective to enable custom themes that can detect and respond to user color-scheme preferences.code/core/src/theming/tests/create.test.js (1)
151-171: LGTM! Tests correctly verify theme resolution.The test suite effectively validates that
themes.normalresolves to eitherthemes.lightorthemes.darkbased on the user's color-scheme preference. The use of dynamic imports withvi.resetModules()ensures each test observes the theme initialization with the mocked preference.code/core/src/manager/globals/exports.ts (1)
678-678: LGTM! Consistent addition to public theming APIs.Adding
getPreferredColorSchemeto all three theming export surfaces (storybook/internal/theming,@storybook/theming,@storybook/core/theming) ensures consistent API availability across different import paths.Also applies to: 705-705, 732-732
code/core/src/theming/create.ts (2)
7-23: Clean refactoring of theme initialization logic.The introduction of the
Themesinterface andthemesBaseconstant improves code organization and makes the dynamic resolution ofthemes.normalexplicit and testable. The approach correctly implements the PR objective of makingthemes.normalfollow the user's system color-scheme preference.
20-23: Resolved — preferredColorScheme is always 'light' | 'dark'; indexing is safe.
getPreferredColorScheme (code/core/src/theming/utils.ts) returns 'light' or 'dark' on all paths (fallback 'light'), so themesBase[preferredColorScheme] cannot be undefined.
I could not repro Thomas's report. Let's let him make a proper bug report if he is able to reproduce his issue consistently on The change made is good as far as I'm concerned. I've added the missing doc, now we just need green CI. |
@Sidnioulz Sorry for the misunderstanding, that was not really a bug report but rather an example of what I was trying to achieve but which would not work because the feature implemented in this PR is not yet in the main branch. As a workaround I'm currently using this until this PR gets merged: import { create as createTheme, themes } from 'storybook/theming'
import logoLight from '../src/assets/images/logo_light.svg?url'
import logoDark from '../src/assets/images/logo_dark.svg?url'
const themeDark = createTheme({
...themes.dark,
brandImage: logoDark,
})
const themeLight = createTheme({
...themes.light,
brandImage: logoLight,
})
/**
* Copy-Pasted and adapted from
* @see https://github.com/storybookjs/storybook/blob/next/code/core/src/theming/utils.ts
*/
function getPreferredColorScheme() {
if (!window || !window.matchMedia) return 'light'
const isDarkThemePreferred = window.matchMedia('(prefers-color-scheme: dark)').matches
if (isDarkThemePreferred) return 'dark'
return 'light'
}
addons.setConfig({
theme: getPreferredColorScheme() === 'dark' ? themeDark : themeLight,
}) |
jonniebigodes
left a comment
jonniebigodes
left a comment
There was a problem hiding this comment.
@elisezhg, thank you for putting this pull request together and helping with the documentation. Appreciate it 🙏 !
Left a small item documentation-wise to be looked into when possible.
cc @Sidnioulz
Hope both of you have a fantastic weekend
Stay safe
Co-authored-by: jonniebigodes <joaocontadesenvolvimento@gmail.com>
9 participants
Closes #28664
What I did
themes.normalto either the light or dark theme, according to the user preferencegetPreferredColorSchemeas suggested in the issue so it can be used in custom themesChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn task --task sandbox --start-from auto --template react-vite/default-tsthemes.normalin yourpreview.ts:Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This pull request introduces dynamic theming based on user preferences and exports a utility function for broader use.
code/core/src/manager/globals/exports.ts: AddedgetPreferredColorSchemeto global exports.code/core/src/theming/create.ts: Introduced dynamic setting ofthemes.normalbased on user preference.code/core/src/theming/index.ts: ExportedgetPreferredColorSchemeutility function.code/core/src/theming/tests/create.test.js: Added tests to verifythemes.normalrespects user preferences.Summary by CodeRabbit