[RFC] Private API prototypes #362

emplums · 2018-11-19T22:21:57Z

This PR is to test out different private API prototypes. This is very WIP and probably won't ship.

I'd like to test the following:

Removing withSystemProps (and thus system-components) and using styled-system + emotion directly in the component file
Removing withSystemProps and thus system-components) and using styled-system + styled-components directly in the component file
- Refactor a fully CSS in JS component with this method
- BorderBox: https://github.com/primer/components/pull/362/files#diff-f4feaf115f7acdc3f36c8bd1fa501ff5
- Box: https://github.com/primer/components/pull/362/files#diff-2ce98dcc0818477f331e426d8528ea2e
- Refactor a component that uses Primer CSS with this method
- Button: https://github.com/primer/components/pull/362/files#diff-0667e450e6d5ec59d1a58311f568c7f5
- Refactor a component with nested components in it with this method
- Flex: https://github.com/primer/components/pull/362/files#diff-536fc7306c4dbbc05c91c9d209661a6f

... more to come

Baseline Success Criteria:

Must be able to extend a component within another component (ex. styled(Box))
Must use styled-system
Can use is prop in the public API to easily change rendered tag
No system props carried over as HTML attributes
Must provide a clean, enjoyable developer experience
Must be able to use themeGet or something similar to use values from a custom provided theme

Nice to have:

Doesn't create any breaking changes to the public API

This works, but props related to CSS are not getting removed from the rendered HTML

vercel · 2018-11-19T22:22:01Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

colebemis · 2018-11-19T23:17:30Z

How would we expose system props on "composite" components like Dropdown without withSystemProps? https://github.com/primer/components/blob/034c19ca1a6406de09ff56518fd4541adb2ebf86/src/Dropdown.js#L13-L52

emplums · 2018-11-19T23:20:48Z

@colebemis in this situation, instead of calling withSystemProps(Dropdown you'd replace that line with:

const styledDropdown = styled(Dropdown)`
${space}
${color}
`

export default styledDropdown

(probably with better naming though 😂 )

colebemis · 2018-11-19T23:36:23Z

src/BorderBox.js

+  borderColor: colors.gray[2],
+  borderRadius: theme.radii[1],
+  theme // set our theme to the default if no theme is provided
+}


These default props won't update if the consumer is using a custom theme, right? Is that a problem?

Not as written, no. I think we still have to figure out what the "right" way is to do a default theme prop, if that's even a thing. 🙁

@shawnbot this is setting the default theme here only if the user doesn't provide one, or are you talking about something else?

@colebemis - yeah, these values won't use the provided theme value it always defaults to our theme values... though I'm not sure if that's a huge problem or not? IMO the default values should be set to values from the Primer theme, and can always be overridden if you'd like to provide your own theme by providing a value for that prop. cc @broccolini

@emplums I think the problem with this approach is that themeGet() doesn't work with it — but I could be wrong, and maybe it will magically work the way I expect it to with styled-components? IIRC, this wasn't working last I checked:

const Blue = styled.div` background-color: ${themeGet('colors.blue.4')} ` Blue.defaultProps = {theme} // <Blue /> → no background-color

Last I remember looking at this, I suspected it was an issue with the theme not being available in context as opposed to props, but I might be totally off base. 🤔

Ah right we'd have to do it like this if we want to use themeGet since props aren't available outside the context of the component: 7e1c2c1

Doesn't feel too bad though?

fwiw this isn't working currently in primer/components anyways 😂

I discovered something interesting... the reason this isn't working in primer/components (more accurately... the docs for primer/components) is because the theme prop for every component rendered in the docs is being overridden by the theme provided to Layout from mdx-docs... so the default is never set in any component because there is already a theme prop provided 🤦‍♀️

Two take-aways from this:

We should really have some sort of clean-ish dev environment so we aren't deciphering between primer/components issues and issues due to clashes with our docs stack 😂

I'd be keen to pair with you @shawnbot tomorrow to pull mdx-docs out and use your system for primer.style/design so that we're on the same page between repos and don't need to worry about theme clashes with Layout.

Yay, nice detective work! I think this is a simple fix: we can just use an <MDXProvider> in place of the Layout. Let me know if that works, and we can pair if need be!

Ah right we'd have to do it like this if we want to use themeGet since props aren't available outside the context of the component: 7e1c2c1

Doesn't feel too bad though?

@emplums and team, I read through this RFC PR, dig it. I apologize if you arrived at a conclusion or answer already regarding the themeGet utility, however, in the off chance that you're still exploring approaches, I have found that you can still leverage themeGet successfully in your custom Primer component when the theme provider is not defined, but the default theme prop is, as follows (ala styled-components)

... themeGet } from 'styled-system' import theme from './theme' ... border: ${props => themeGet('borders.1'), theme.borders[1])(props)}; ... Box.defaultProps = { theme } ...

@mannybecerra ah yes! We found a bug that was preventing the defaultProps from kicking in, which is why we were having issues with themeGet. We've resolved that and now it should work swimmingly! 🙌

renderPage

src/Box.js

colebemis · 2018-11-27T01:26:42Z

src/Flex.js

+  ${FLEX_CONTAINER}
+  ${COMMON}
+  ${display}
+`


Is there a reason we use template literal syntax instead of passing the system prop functions as arguments?

const Flex = styled.div` ${FLEX_CONTAINER} ${COMMON} ${display} ` // vs const Flex = styled.div(FLEX_CONTAINER, COMMON, display)

I'll be honest, I prefer the latter form for components that only use system props. However, I think there's a lot of value in consistency, and if folks need to add additional styles, it should be obvious how to do that. So leaving all of the components using tagged template literals might be good for that.

OTOH: in theory, there's a performance penalty for using template literals when we don't need them. Maybe we could compile those away with a babel plugin or something? 🤔

@colebemis @shawnbot hmm yeah honestly the only reason I left the syntax like that was because I'd previously had some CSS in there 😂 I like having consistent syntax but if there's a performance penalty then I don't think doing const Flex = styled.div(FLEX_CONTAINER, COMMON, display) is too confusing of a departure from the tagged template literal syntax

Chiming in here 👋 since I know a little bit about styled-components 🙈: our Babel plugin automatically optimizes tagged template literals under the hood for perf. For example, by default babel-preset-env (or whatever the relevant Babel plugin is that's used under the hood) would turn this component:

const Simple = styled.div` width: 100%; `

into this JS:

var _templateObject = _taggedTemplateLiteral(['width: 100%;'], ['width: 100%;']) function _taggedTemplateLiteral(strings, raw) { return Object.freeze(Object.defineProperties(strings, { raw: { value: Object.freeze(raw) } })) } var Simple = _styledComponents2.default.div(_templateObject)

which is done to be 100% spec-compliant, most of which is 100% irrelevant for writing styled components.

Our Babel plugin pre-empts that transformation and instead turns it into this much faster and smaller version ✨:

var Simple = _styledComponents2.default.div(['width: 100%;'])

You can read more about that transformation in the docs, but TL;DR: if you use the Babel plugin it's not a problem since the two forms are basically identical perf-wise, if not then... use the Babel plugin! 😉

emplums · 2018-12-03T21:14:52Z

Closing this since I've started the refactor in #368

without system-components

d16d252

This works, but props related to CSS are not getting removed from the rendered HTML

emplums added the status: wip label Nov 19, 2018

emplums changed the title ~~[WIP WIP WIP WIP WIP] Private API prototypes~~ [RFC] Private API prototypes Nov 19, 2018

colebemis reviewed Nov 19, 2018

View reviewed changes

emplums force-pushed the prototypes branch from 6a97337 to 6ca7712 Compare November 20, 2018 21:45

Using styled-components

218d6bd

renderPage

emplums force-pushed the prototypes branch from 6ca7712 to 218d6bd Compare November 20, 2018 21:48

Emily added 10 commits November 20, 2018 15:10

Using themeGet for defaults

7e1c2c1

combine theme + docs theme together in Layout

ba16801

add emotion SSR back

9695945

set defaults in defaultProps

9e50362

Merge branch 'master' into prototypes

c06d01e

refactor Box to be a styled component

8d215bd

clean up BorderBox imports

2cfb719

remove theme file

8a0f18b

lint

6874888

add theme to Box defaultProps

303b95f

shawnbot reviewed Nov 24, 2018

View reviewed changes

src/Box.js Outdated Show resolved Hide resolved

Emily added 6 commits November 26, 2018 15:52

update box tests

ddf9d3f

DRY IT ALL UP

5038673

simplify Box

fff3087

add COMMON to Box

7d5d56b

convert Flex

af7552b

refactor Button

fad8e3d

colebemis reviewed Nov 27, 2018

View reviewed changes

Emily added 2 commits November 27, 2018 11:11

update tests

2623240

lint

ee60e11

emplums closed this Dec 5, 2018

emplums deleted the prototypes branch January 25, 2019 20:27

[RFC] Private API prototypes #362

[RFC] Private API prototypes #362

Uh oh!

Conversation

emplums commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Baseline Success Criteria:

Nice to have:

Uh oh!

vercel bot commented Nov 19, 2018

Uh oh!

colebemis commented Nov 19, 2018

Uh oh!

emplums commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emplums Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emplums Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mannybecerra Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxstbr Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emplums commented Dec 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

emplums commented Nov 19, 2018 •

edited

Loading

emplums commented Nov 19, 2018 •

edited

Loading

emplums Nov 20, 2018 •

edited

Loading

emplums Nov 20, 2018 •

edited

Loading

mannybecerra Nov 24, 2018 •

edited

Loading

mxstbr Nov 28, 2018 •

edited

Loading