Skip to content

Conversation

@codecaaron
Copy link
Contributor

@codecaaron codecaaron commented Feb 26, 2021

Overview

Our styling needs are becoming a bit more complex in the near term. There are current use cases for global css variables and near term uses for different Color Palettes (LE colors) + Modes (light and dark) that are going to be tricky to scale.

This PR Introduces them in a controlled way by creating a single point of entry for any Global styles through a new component that wraps the default emotion ThemeProvider. While also adding the first CSS vars to the theme headerHeight.

Adding support for CSS variables has a few huge advantages:

  • Semantic meaning (e.g. 4rem can mean a lot of things but var(--headerHeight) cannot.
  • Scopable to elements and screensizes (e.g. local color modes only effect their direct descendants).
  • Invisible to implementation (e.g. any JS that is consuming these doesn't need to be aware of its existence).

Adding any new pattern like this is dangerous, and I'm suggesting we consider using the following best practices:

  • Root namespace vars should have a single point of entry (at the level of the root GamutThemeProvider).
  • No mutation of root variables downstream.
  • Components down in the stack should not use Global in almost all circumstance (the current header should consume the variable not provision it).
  • Overriding root variables (colors should have a clear and consistent patterns e.g. { //markup }) and are scoped to the component.
  • Variables must be accessible in JS (preferably through theme).

To ensure we can safely introducing these this PR adds variable serialization through createThemeVariables and createVariables.

createThemeVariables Usage

Basic example:

const { theme, vars } = createThemeVariables(rawTheme, ['keys', 'to' 'serialize']);

Contrived example:

// input 
const theme = {
  elements: {
     headerHeight: { base: '48px', xs: '64px' } , 
     contentWidth: '1024px' 
   }
}

// output theme
const theme = {
  elements: { 
    headerHeight: 'var(--headerHeight)',  
    contentWidth: 'var(--headerHeight)' }
}

// output vars
const vars = {
  '--headerHeight': '48px',
  '--contentWidth': '1024px',
  '@media screen and (min-width: 767px)`: {
       '--headerHeight': '64px',
  }
}

The returned theme object will include valid references to these global variables to be used through out the app without any knowledge of their complexity. End usage of these variables remains the same down stream. For almost all use cases this provides a more flexible + robust method of distributing styles combining the typesafety and dynamism of TS and the performance + flexibility of CSS Variables

const MyComponent = styled.div`
 height: ${({ theme }) => theme.elements.headerHeight};  // yay typesafe CSS vars
`

createVariables usage

Takes any object and returns a CSSObject with variable definitions along any selector.

const myVars = createVariables({ fontColor: {  base: 'blue',  xs:  'green', '[disabled]': 'grey' }})

const output = {
  `--fontColor`: 'blue',
  '@media min xxx': {
        `--fontColor`: 'green',
   },
  '[disabled]': {
      `--fontColor`: 'grey',
    }

Real world:

import { createVariables } from '@codecademy/gamut-styles';

const ColorMode = styled.div(({ mode, theme }) => css(createVariables(theme[mode])));

const LightItUp = ({ children }) => <ColorMode mode="light">{children}</ColorMode>
  • Adds GamutThemeProvider with initial Global styles.
  • Adds createThemeVariables and createVariables for simple CSS Variable serialization.

PR Checklist

  • Related to designs:
  • Related to JIRA ticket: ABC-123
  • I have run this code to verify it works
  • This PR includes unit tests for the code change

@codecaaron codecaaron marked this pull request as ready for review February 26, 2021 23:46
@codecaaron codecaaron requested a review from a team as a code owner February 26, 2021 23:46
@codecaaron codecaaron requested review from JoshuaKGoldberg, MattSchiller, christian-dinh, dreamwasp and jakemhiller and removed request for a team February 26, 2021 23:46
Comment on lines +11 to +12
const createMediaQuery = (size: MediaSize, direction: 'min' | 'max') =>
`@media only screen and (${direction}-width: ${breakpoints[size]})`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes new line characters from gunking up the strings. This is a no op.

Copy link
Contributor

Choose a reason for hiding this comment

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

imo, a small comment/warning about that would be helpful for future us's

Copy link
Contributor

@MattSchiller MattSchiller left a comment

Choose a reason for hiding this comment

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

I am perhaps highly biased, being in the discussions about this since Christian's PRs, but I think this is a fantastic first PR. I'm curious/excited to see where this goes, how it evolves, and where it takes us.

Thanks, Aaron!

Comment on lines +11 to +12
const createMediaQuery = (size: MediaSize, direction: 'min' | 'max') =>
`@media only screen and (${direction}-width: ${breakpoints[size]})`;
Copy link
Contributor

Choose a reason for hiding this comment

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

imo, a small comment/warning about that would be helpful for future us's

Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🌮

(will post comment requests soon, we're still pairing)

Copy link
Contributor

@christian-dinh christian-dinh left a comment

Choose a reason for hiding this comment

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

ya this is sick gg 🥇

Copy link

@casey-speer casey-speer left a comment

Choose a reason for hiding this comment

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

So to handle say, LE dark vs light mode, we'd want our theme to be something like

const theme = {
  leDarkMode: {
    bgColor: 'black'
  },
  
  leLightMode: {
    bgColor: 'white'
  }
}

And do

createThemeVars(theme, ['leDarkMode', 'leLightMode']);

to serialize?

In our use case, would we want to keep this outside gamut for the near term (ie, create our own LEThemeProvider or add a vars argument to withEmotion or some other similar approach) or is the idea to start adding all vars in gamut?

Another question — if for whatever reason we wanted to provide extensions / variations of a theme somewhere in the monolith component tree, is it acceptable to nest <ThemeProvider>s ? (Not sure if emotion/styled system has any special support for that).

Aside from the small comments here, looks good to me overall.

@codecaaron
Copy link
Contributor Author

@casey-speer

I've updated this to 2 separate methods: createVariables and

  • createThemeVariables: used to redefine theme and generate global variables, not exported.
  • createVariables: takes any object and returns a CSSObject with variable definitions along any selector. Exported.

For your use case you will want to use createVariables.

import { createVariables } from '@codecademy/gamut-styles';
import styled from '@emotion/styled';

const ColorMode = styled.div(({ mode, theme }) => css(createVariables(theme[mode])));

const Scene = ({ children }) => <ColorMode mode="light">{children}</ColorMode>

@christian-dinh
Copy link
Contributor

I regret to inform everyone that this concept is now a Mark Dalgleish joke tweet

https://twitter.com/markdalgleish/status/1366629835880075267

@codecaaron codecaaron requested a review from slin12 March 2, 2021 14:56
Copy link
Contributor

@slin12 slin12 left a comment

Choose a reason for hiding this comment

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

😍

@codecaaron codecaaron added the Ship It :shipit: Automerge this PR when possible label Mar 4, 2021
@codecademydev
Copy link
Collaborator

📬Published Alpha Packages:

@codecademy/gamut-illustrations@0.7.5-alpha.c3d839.0
@codecademy/gamut-labs@10.4.4-alpha.c3d839.0
@codecademy/gamut-styles@8.2.3-alpha.c3d839.0
@codecademy/gamut-tests@2.1.6-alpha.c3d839.0
@codecademy/gamut@25.5.1-alpha.c3d839.0
@codecademy/markdown-overrides@0.5.7-alpha.c3d839.0
@codecademy/styleguide@29.6.10-alpha.c3d839.0

@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #1444 (c3d8394) into main (b1792d8) will increase coverage by 42.70%.
The diff coverage is 86.66%.

@@             Coverage Diff             @@
##             main    #1444       +/-   ##
===========================================
+ Coverage   40.31%   83.01%   +42.70%     
===========================================
  Files         331      336        +5     
  Lines        2704     2755       +51     
  Branches      696      709       +13     
===========================================
+ Hits         1090     2287     +1197     
+ Misses       1434      415     -1019     
+ Partials      180       53      -127     
Impacted Files Coverage Δ
packages/gamut-styles/src/theme/props.ts 100.00% <ø> (ø)
packages/gamut-styles/src/theme/theme.tsx 100.00% <ø> (ø)
...s/styleguide/.storybook/addons/system/enhancers.ts 0.00% <ø> (ø)
...es/styleguide/.storybook/addons/system/propMeta.ts 0.00% <0.00%> (ø)
...uide/.storybook/components/PropsTable/constants.ts 100.00% <ø> (+100.00%) ⬆️
...es/gamut-styles/src/theme/utils/createVariables.ts 76.92% <76.92%> (ø)
...mut-styles/src/theme/utils/createThemeVariables.ts 95.00% <95.00%> (ø)
...ages/gamut-styles/src/theme/GamutThemeProvider.tsx 100.00% <100.00%> (ø)
.../gamut-styles/src/theme/utils/shouldForwardProp.ts 100.00% <100.00%> (ø)
packages/gamut-styles/src/variables/elements.ts 100.00% <100.00%> (ø)
... and 169 more

@github-actions github-actions bot merged commit f4fd13c into main Mar 4, 2021
@github-actions github-actions bot deleted the ar-serialized-vars branch March 4, 2021 21:28
@github-actions github-actions bot removed the Ship It :shipit: Automerge this PR when possible label Mar 4, 2021
@codecademydev
Copy link
Collaborator

🚀 Styleguide deploy preview ready!

https://604150fe292be9130c20716e--gamut-preview.netlify.app

Deploy Logs

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.

8 participants