-
Notifications
You must be signed in to change notification settings - Fork 645
[RFC] Private API prototypes #362
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
Changes from all commits
d16d252
218d6bd
7e1c2c1
ba16801
9695945
9e50362
c06d01e
8d215bd
2cfb719
8a0f18b
6874888
303b95f
ddf9d3f
5038673
fff3087
7d5d56b
af7552b
fad8e3d
2623240
ee60e11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,23 @@ | ||
| import {withSystemProps} from './system-props' | ||
| import styled from 'styled-components' | ||
| import Box from './Box' | ||
| import theme from './theme' | ||
| import {BORDER} from './constants' | ||
|
|
||
| const BorderBox = withSystemProps( | ||
| { | ||
| is: Box, | ||
| border: 1, | ||
| borderColor: 'gray.2', | ||
| borderRadius: 1 | ||
| }, | ||
| ['boxShadow'] | ||
| ) | ||
| const BorderBox = styled(Box)` | ||
| ${BORDER}; | ||
| ` | ||
|
|
||
| BorderBox.defaultProps = { | ||
| theme, | ||
| border: '1px solid', | ||
| borderColor: 'gray.2', | ||
| borderRadius: 1 | ||
| } | ||
|
|
||
| // spread prop types here | ||
| BorderBox.propTypes = { | ||
| ...Box.propTypes | ||
| ...Box.propTypes, | ||
| ...BORDER.propTypes | ||
| } | ||
|
|
||
| export default BorderBox | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,18 @@ | ||
| import {withSystemProps, LAYOUT, COMMON} from './system-props' | ||
| import {LAYOUT, COMMON} from './constants' | ||
| import styled from 'styled-components' | ||
| import theme from './theme' | ||
|
|
||
| const Box = withSystemProps('div', [...LAYOUT, ...COMMON]) | ||
| const Box = styled.div` | ||
| ${LAYOUT} ${COMMON}; | ||
| ` | ||
|
|
||
| Box.defaultProps = { | ||
| theme | ||
| } | ||
|
|
||
| Box.propTypes = { | ||
| ...LAYOUT.propTypes, | ||
| ...COMMON.propTypes | ||
| } | ||
|
|
||
| export default Box |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,22 @@ | ||
| import {withSystemProps, FLEX_CONTAINER, COMMON, FLEX_ITEM} from './system-props' | ||
| import styled from 'styled-components' | ||
| import {display} from 'styled-system' | ||
| import theme from './theme' | ||
| import {FLEX_CONTAINER, FLEX_ITEM, COMMON} from './constants' | ||
|
|
||
| const Flex = withSystemProps( | ||
| { | ||
| is: 'div', | ||
| display: 'flex' | ||
| }, | ||
| [...FLEX_CONTAINER, ...COMMON] | ||
| ) | ||
| const Flex = styled.div` | ||
| ${FLEX_CONTAINER} | ||
| ${COMMON} | ||
| ${display} | ||
| ` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? 🤔
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! 😉 |
||
|
|
||
| Flex.Item = withSystemProps('div', [...FLEX_ITEM, ...COMMON]) | ||
| Flex.defaultProps = { | ||
| display: 'flex', | ||
| theme | ||
| } | ||
|
|
||
| const FlexItem = styled.div` | ||
| ${FLEX_ITEM} ${COMMON}; | ||
| ` | ||
| Flex.Item = FlexItem | ||
|
|
||
| export default Flex | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These default props won't update if the consumer is using a custom
theme, right? Is that a problem?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not as written, no. I think we still have to figure out what the "right" way is to do a default
themeprop, if that's even a thing. 🙁Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:Last I remember looking at this, I suspected it was an issue with the
themenot being available in context as opposed to props, but I might be totally off base. 🤔Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right we'd have to do it like this if we want to use
themeGetsince props aren't available outside the context of the component: 7e1c2c1Doesn't feel too bad though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw this isn't working currently in primer/components anyways 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discovered something interesting... the reason this isn't working in primer/components (more accurately... the docs for primer/components) is because the
themeprop for every component rendered in the docs is being overridden by the theme provided toLayoutfrommdx-docs... so the default is never set in any component because there is already athemeprop 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emplums and team, I read through this RFC PR, dig it. I apologize if you arrived at a conclusion or answer already regarding the
themeGetutility, however, in the off chance that you're still exploring approaches, I have found that you can still leveragethemeGetsuccessfully in your custom Primer component when the theme provider is not defined, but the defaultthemeprop is, as follows (ala styled-components)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mannybecerra ah yes! We found a bug that was preventing the
defaultPropsfrom kicking in, which is why we were having issues withthemeGet. We've resolved that and now it should work swimmingly! 🙌