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(labs): Fix theming implementation #360

Merged
merged 9 commits into from
Dec 12, 2019

Conversation

anicholls
Copy link
Contributor

@anicholls anicholls commented Dec 10, 2019

Summary

There were two issues with #272:

  • During review, we switched from using deepmerge to lodash/merge. I didn't realize at the time that lodash/merge mutates the object it's returning, so we were updating the defaultCanvasTheme unintentionally
  • I tried using our own ThemeProvider 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 custom styled function and how React hooks work. Since we expect theme to be passed to useTheme through styled from Emotion (since we can't use useContext within class components), we do need to use Emotion's ThemeContext. 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 wrapping styled function will not work with a custom context.

Checklist

  • branch has been rebased on the latest master commit
  • yarn test passes
  • all (dev)dependencies that the module needs is added to its package.json
  • code has been documented and, if applicable, usage described in README.md

@cypress
Copy link

cypress bot commented Dec 10, 2019



Test summary

56 0 0 0


Run details

Project canvas-kit
Status Passed
Commit eb2a61f
Started Dec 12, 2019 8:00 PM
Ended Dec 12, 2019 8:01 PM
Duration 01:15 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 61

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

Copy link
Contributor

@lychyi lychyi left a 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.

@anicholls anicholls merged commit 2bafc78 into Workday:master Dec 12, 2019
@anicholls anicholls deleted the fix-theming branch December 12, 2019 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants