Introduce emotion and styled-system #164
Conversation
src/Block.js
Outdated
| 'minWidth', | ||
| 'width', | ||
| 'zIndex' | ||
| ) |
There was a problem hiding this comment.
Uhhh so yeah, there's zero implementation in this component anymore. It's purely system props! 😱
| } | ||
| } | ||
| return keys | ||
| }, []) |
There was a problem hiding this comment.
This function is silly, and we'll want to put this somewhere that other components can use it. But for now, this is how we generate the list of all themed values for our color props (bg, borderColor, and color): white, black, gray.0, blue.5, et al.
examples/_app.js
Outdated
| <Block p={3}>{render()}</Block> | ||
| </div> | ||
| <ThemeProvider theme={theme}> | ||
| <div className="text-dark-gray"> |
There was a problem hiding this comment.
This should probably just be <Block color='bodytext'> (or <Block color='gray.9'>).
examples/component-examples/Block.js
Outdated
| @@ -4,7 +4,21 @@ import {LiveEditor} from '@compositor/kit' | |||
| import theme from '../../src/theme' | |||
| import {Block, Text, Heading} from '../../src' | |||
|
|
|||
| const colors = Object.keys(theme.colors.bg) | |||
| const Mono = props => <Block fontFamily="mono" {...props} /> | |||
There was a problem hiding this comment.
This was repeated a lot in the examples, so I made a little component out of it.
src/Text.js
Outdated
| 'fontFamily', | ||
| 'fontWeight', | ||
| 'lineHeight', | ||
| 'space' |
There was a problem hiding this comment.
Again: zero implementation in here!
There was a problem hiding this comment.
This should have fontSize too.
We don't need system-components! It's optional, and probably better to start with understanding the mechanics of how emotion and styled-system work, this is from the styled-system docs:
|
Understood. However, what I like a lot about it is that it reduces (if not eliminates) boilerplate code for components that use only system props. I haven't tried implementing anything but Block or Text yet, though, so maybe we'll see what that looks like next? |
src/Block.js
Outdated
| 'fontFamily', | ||
| 'fontSize', | ||
| 'fontWeight', | ||
| 'lineHeight', |
There was a problem hiding this comment.
Nix fontFamily through lineHeight here; they're not needed in Block.
|
Just to follow up on the system-components thing: I'm going to look into what it looks like to use styled-system with emotion, both with and without tagged template literals. I'll update this PR so we can compare the approaches in our session tomorrow. |
src/Block.js
Outdated
| 'boxShadow', | ||
| 'color', | ||
| 'display', | ||
| 'fontFamily', |
There was a problem hiding this comment.
Can we remove the typography styles from here? I don't think they should be part of the Block component.
| @@ -5,7 +5,8 @@ const Box = props => <Block {...props} /> | |||
|
|
|||
| Box.defaultProps = { | |||
There was a problem hiding this comment.
Since Block with system props does all this, can we deprecate this component and call "Block" => "Box" again? If that's easier to do in a follow up that's 🆒 .
src/Text.js
Outdated
| 'fontFamily', | ||
| 'fontWeight', | ||
| 'lineHeight', | ||
| 'space' |
There was a problem hiding this comment.
This should have fontSize too.
src/theme.js
Outdated
| @@ -1,53 +1,24 @@ | |||
| const black = '#1b1f23' | |||
There was a problem hiding this comment.
I guess this repeats primer-primitives, makes sense to live here but we should figure out what to do with that.
There was a problem hiding this comment.
Yeah, it'd be nice to get a lot of this stuff into primer-primitives and make any theme tweaks that we need for primer-react here, rather than recreating the entire thing.
src/theme.js
Outdated
| } | ||
|
|
||
| const {blue, gray, green, purple, red, yellow} = colors | ||
| const {blue, green, orange, purple, red, yellow} = colors | ||
|
|
||
| colors.border = { |
There was a problem hiding this comment.
Let borders have all the colors rather than limiting them to our previous css variations.
There was a problem hiding this comment.
Yeah, the colors.border object is a holdover. See how it's been factored out in #157.
src/theme.js
Outdated
| ]) | ||
| }, | ||
| lineHeights: { | ||
| normal: 1.5 |
There was a problem hiding this comment.
We should have 1 and 1.25 here too.
There was a problem hiding this comment.
Yep. condensed and ultra-condensed, right?
There was a problem hiding this comment.
@shawnbot yup! I was considering that people may want to pass in a numeric value, but I'd prefer to encourage using these pre-defined line-heights since they work with our typography scale. I think for custom non-system things like that they should use the css prop (when we add it).
| expect(rendersClass(<Flash m={4} />, 'm-4')).toEqual(true) | ||
| // TODO: understand why these aren't working | ||
|
|
||
| xit('respects margin utility prop', () => { |
There was a problem hiding this comment.
What does the 'x' here do? Is it just silencing this test?
There was a problem hiding this comment.
Yeah, "marks it as pending".
Fix tests and execution failures in Caret, CaretBox, and CircleOcticon
Updating MergeStatus.js to styled-systems emotion
Upgrading Link.js to styled-system
|
We're going to change the base of this to a forthcoming v1 PR. 🌮 🎉 |
shawnbot
changed the title
This
is an attempt tobootstraps primer-react with emotion and styled-system and puts them to work for the Block and Box components. Tests and linting will likely fail spectacularly, but the diff is the main attraction here.Some notes:
emotionandstyled-system,emotion-themingfor the ThemeProvider,system-componentsto create components entirely from system props, andreact-emotion(which system-components uses, but it's not clear to me what the diff is withemotion).bordersarray to the theme to supportborder={1}and the directional border props.shadowsobject to the theme to support theboxShadowprop.Box.defaultPropsto match the requisite system props values.