-
Notifications
You must be signed in to change notification settings - Fork 221
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(labs): Fix theming implementation #360
Conversation
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
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.
It may be worth writing a simple test but let's get this into master first.
e25d38f
to
45fcdeb
Compare
Summary
There were two issues with #272:
deepmerge
tolodash/merge
. I didn't realize at the time thatlodash/merge
mutates the object it's returning, so we were updating thedefaultCanvasTheme
unintentionallyThemeProvider
to mitigate an issue with@storybook/addon-knobs
. After realizing this wasn't causing the problem with the addon, I left it as is. However, there is an issue with this in conjunction with our customstyled
function and how React hooks work. Since we expecttheme
to be passed touseTheme
throughstyled
from Emotion (since we can't useuseContext
within class components), we do need to use Emotion'sThemeContext
. This is because Emotion is calling our interpolations and passing the theme through (using their context, without the option to override). If we ever move to functional components exclusively in the future, we can change this to use a custom context. Otherwise our current approach with a wrappingstyled
function will not work with a custom context.Checklist
yarn test
passespackage.json