-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(storybook): add theme toggle to storybook #18123
Conversation
aac7fb0
to
af21858
Compare
📸 UI snapshots have been updated10 snapshot changes in total. 0 added, 10 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super cool. Clicked around locally and everything seemed to work as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this needs to be changed (or removed) as we now do this via the theme picker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think for the purpose of snapshots we still need the decorator.
BTW looks like the now-unused dark mode and light mode snapshots weren't deleted, we might want to do some pruning of unused snapshots at some point. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'd like to address this by adding an option to testOptions
where you can specify wether "legacy", "3000" or "all" themes should be captured.
I was hoping Storybook would provide a way to set globals for a specific story, so that we could override the theme there, but that doesn't seem to be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, but we do need to keep the 3000 navigation stories, since the story we have now just shows the classic navigation
# Conflicts: # frontend/__snapshots__/scenes-app-notebooks--headings.png # frontend/__snapshots__/scenes-app-notebooks--recordings-playlist.png
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
I'm forcing these stories to use the feature flag now (see visual regression tests). Also I've added one for the "base 3000 nav". |
# Conflicts: # frontend/__snapshots__/posthog-3000-sidebar--feature-flags.png
Problem
We want to test components in all theme variants with storybook.
Changes
This PR adds a theme toggle via Storybook globals to Storybook. The toggle code is not super clean (it also doesn't use
themeLogic
and instead sets the theme by updating the theme property onbody
accordingly), but works in most cases.How did you test this code?
Ran Storybook locally