[POC] Emotion Theming #382
Conversation
| @@ -146,6 +147,7 @@ | |||
| "peerDependencies": { | |||
| "@emotion/core": "10.0.35", | |||
| "@emotion/styled": "10.0.27", | |||
| "emotion-theming": "10.0.27", | |||
There was a problem hiding this comment.
We've run into compatibility issues in the past in applications that use our library if there's an @emotion version mismatch, see #323. Basically, styles don't always apply as expected, which resulted in (mostly) padding/alignment issues.
It's higher maintenance to handle it this way but I think it is worth the small cost.
| "@react-aria/focus": "^3.0.2", | ||
| "emotion-theming": "10.0.27", |
There was a problem hiding this comment.
We will have to update our Usage documentation to note that all applications using radiance-ui need to set up a ThemeProvider at the top level of their App--or at least at the highest-level that makes use of radiance-ui components: https://emotion.sh/docs/theming
| @@ -1,7 +1,16 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`<Toggle /> UI snapshot renders the component 1`] = ` | |||
| .emotion-2 { | |||
| Array [ | |||
There was a problem hiding this comment.
Ignore the snapshot changes--immaterial.
| }; | ||
|
|
||
| const Toggle = ({ checked, label, onChange }) => { | ||
| const [theme, setTheme] = React.useState(lightTheme); |
There was a problem hiding this comment.
This is, of course, not how we'd actually use the theming. But this component was used as an example because:
- It has clearly distinct colors (a lot of our components are actually just whitespace, so the theming isn't as apparent).
- It relies on a third party component to which we're passing colors as props, so not only do we have to account for theming in our
style.tsfiles but we need to handle the props we're passing in the component logic. - It highlights a pattern we need to move away from, e.g. using specific colors like
COLORS.purple15. We will need to migrate all specific colors over to their themed-generics (e.g.primaryTint5, which is something I made up, by the way) so that we can make use of different themes. I would not describe the existing color setup asspaghetti, but I would say that our usage of aliases for the colors is very incomprehensive at the moment: https://github.com/curology/radiance-ui/blob/ffa06cfa2f03dea0e676965cbad0ea90c06603eb/src/constants/colors/index.ts
There was a problem hiding this comment.
Good callout on #3 . I wonder how many places are using color constants that are too descriptive about the color itself.
| @@ -17,7 +17,7 @@ export const Container = styled.div` | |||
| `; | |||
|
|
|||
| export const Label = styled.span` | |||
| color: ${COLORS.primaryTint1}; | |||
| color: ${({ theme }) => theme.COLORS.primaryTint1}; | |||
There was a problem hiding this comment.
We don't need to explicitly pass the themes as props in usage--the ThemeProvider handles it for us.
…COLORS functionality
|
Closing in favor of #387 because CI has gotten out of whack and refuses to build. TODO: Handle fast-forwarding when other branches are merged into master more gracefully. |
We will be incorporating a second theme in
radiance-uishortly. This PR is a POC for the functionality. It pulls in only theTogglecomponent story and adds very minimal theming support for it, as well as a button toggle in the story to swap between the themes.This POC PR does not explore Storybook add-ons that help you easily toggle between themes. You can do a search for "theme" in the Community Add-ons section of Storybook (link) but none seem to have all too much usage, so perhaps better to not add the dependency debt. Dealing with how to best document our themes is something we can figure out later.
EDIT: I've added a button to the top of the story to switch Themes. It works (for now).
You can play around with the review app here: https://curology-radiance-pr-382.herokuapp.com/?path=/story/toggle--usage
EDIT: The review app is broken due to additional code changes--please pull it in locally if you'd like to play with it.