Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 677
[Accepted] ADR: Use Box + sx to author components#2020
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
2f936703514352913d99f619f7f2191315f72fa0dbb01cf7ba591c2567586e8c40fb77323d78c845e32aFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| # ADR 005: Use Box as a building block for all other components | ||
| ## Status | ||
| Approved | ||
| ## Context | ||
| In Primer React and consuming applications, we use many different patterns for creating React components. Two common patterns are: | ||
| 1. Creating components with styled-components | ||
| ```tsx | ||
| const Avatar = styled.img.attrs<StyledAvatarProps>(props => ({ | ||
| height: props.size, | ||
| width: props.size | ||
| }))<StyledAvatarProps>` | ||
| display: inline-block; | ||
| overflow: hidden; | ||
| line-height: ${get('lineHeights.condensedUltra')}; | ||
| border-radius: ${props => getBorderRadius(props)}; | ||
| ${sx} | ||
| ` | ||
| ``` | ||
| [Show full code example →](https://github.com/primer/react/pull/2019/files?diff=split&w=0) | ||
| 2. Creating components with Box | ||
| ```tsx | ||
| const Avatar: React.FC<AvatarProps> = ({size = 20, alt = '', square = false, sx = {}, ...props}) => { | ||
| const styles:BetterSystemStyleObject = { | ||
| display: 'inline-block', | ||
| overflow: 'hidden', | ||
| lineHeight: 'condensedUltra', | ||
| borderRadius: getBorderRadius({size, square}) | ||
| } | ||
| return ( | ||
| <Box | ||
| as="img" | ||
| alt={alt} | ||
| sx={merge<BetterSystemStyleObject>(styles, sx)} // styles needs to merge with props.sx | ||
| {...props} | ||
| /> | ||
| ) | ||
| } | ||
| ``` | ||
| [Show full code example →](https://github.com/primer/react/pull/2019/files?diff=split&w=0) | ||
| | ||
| ## Decision | ||
| Prefer using method #2: Creating components with Box for the following reasons: | ||
| - Better authoring experience with Typescript. With Box, we can improve the API and autocomplete for consuming primitives. [See research](https://github.com/github/primer/discussions/755#discussioncomment-2318144) | ||
| - The styling library (i.e. styled-components) becomes an implementation detail and can be changed later with minimal breaking changes for consumers. (Avoids leaky abstractions) | ||
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. In what cases what
| ||
| - We have had issues with exporting types, we can increase confidence by keeping the exported types close to what we author. | ||
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. Can you expand on the issues we've had with exporting types? MemberAuthor 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. sorry for my lazy phrasing here 😅 What I'm trying to convey here is that
I feel more confident having more control over what we export + keeping it as flat as possible. Types deserves an ADR of it's own, to be honest.
| ||
| See diff for moving Avatar from approach 1 to 2: https://github.com/primer/react/pull/2019/files?diff=split&w=0 | ||
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, this is good to know...I am building new components in dotcom right now, but I suppose if we wanted them upstreamed into Primer React, it would make more sense to follow this guidance!
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'd recommend using the same method in dotcom as well.
We want to improve the way product specific React components can consume primer primitives and those improvements will ship in Box. (more here)
Also, replacing styled-components with a more performant styling framework is on our radar, although not soon.