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

[POC] Emotion Themes #387

Closed
wants to merge 12 commits into from
Closed

[POC] Emotion Themes #387

wants to merge 12 commits into from

Conversation

michaeljaltamirano
Copy link
Contributor

@michaeljaltamirano michaeljaltamirano commented Sep 22, 2020

We will be incorporating a second theme in radiance-ui shortly. This PR is a POC for the functionality. It pulls in only the Toggle component 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.

In the meantime, I've just added a button to the top of the story to switch themes.

The review app can be found here: https://curology-radiance-pr-387.herokuapp.com/?path=/story/*

}
`;

export const AccordionBox = styled.div<{
noBorder: boolean;
isOpen: boolean;
disabled: boolean;
theme?: ThemeType;
Copy link
Contributor Author

@michaeljaltamirano michaeljaltamirano Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emotion 10 recommends exporting a typed styled file to allow us to pass the Theme into styled.div implicitly: https://emotion.sh/docs/typescript

However, this breaks our jest tests when wrapped with a ThemeProvider. Emotion 11 will introduce more configuration to handle this more gracefully (docs, issue comment) but has not yet been released.

Apparently Emotion 11 is supposed to be stable, though, despite no release, so that is an option to consider if the verbosity is painful.

@@ -0,0 +1,60 @@
import PropTypes from 'prop-types';

import COLORS from '.';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/constants/colors/index.ts is unchanged to keep the diff to a minimum, but introducing full theming support would require making changes to that file as well. We'd want to disallow access to COLORS directly except under specific circumstances.

I think our goal should be to make opting-in to the Theme as easy as possible. Personally I've already enjoyed not needing to import COLORS and just access it (and already typed!) off of a theme.

@michaeljaltamirano
Copy link
Contributor Author

Closing in favor of #466

@michaeljaltamirano michaeljaltamirano deleted the poc/emotion-themes branch January 4, 2021 19:38
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