-
Notifications
You must be signed in to change notification settings - Fork 648
ADR: Styling with css #3080
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
ADR: Styling with css #3080
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| # ADR 16: Styling with CSS | ||
|
|
||
| ## Status | ||
|
|
||
| | Stage | Status | | ||
| | -------- | ----------------------------- | | ||
| | Approved | ✅ | | ||
| | Adopted | 🚧 _TODO: add tracking issue_ | | ||
|
|
||
| ## Context | ||
|
|
||
| Reasons for change: Performance, utilise css variables | ||
|
|
||
| The main reason for changing our styling architecture is to remove performance issues for applications: | ||
|
|
||
| Relevant parts from [primer/discussions#1928](https://github.com/github/primer/discussions/1928#reasons): | ||
|
|
||
| 1. Initial page loads take longer | ||
|
|
||
| Because styles are injected during runtime, we can not parallelize this work. The page is un-styled till the javascript bundle is | ||
| downloaded and processed. The processing time is longer on slower devices. | ||
|
|
||
| Benchmarks: | ||
|
|
||
| i. Lab benchmarks: rendering a page with 1000 components | ||
| runtime injection of css: 242ms | ||
| static .css files: 96ms (60% faster) | ||
|
|
||
| ii. Application benchmarks: render times for repo directory (>1000 files, [gh/gh#241090](https://github.com/github/github/pull/241090)) | ||
| with primer/react Box: 3800ms | ||
| with custom css: 3076ms (20% faster) | ||
|
|
||
| 2. Slow server side rendering | ||
|
|
||
| Some applications are able to make up for slower initial render times by collecting styles on the server first. In the correct stack, collecting styles requires rendering the app twice before we can ship any html to the user. | ||
|
|
||
| benchmarks: in the issues-app experiment, collecting styles on the server contributed to + 20% of rendering time (450ms on top of 1900ms) | ||
|
|
||
| 3. Style updates are expensive | ||
|
|
||
| The slowest part of the current stack is updating styles based on props or state. Big changes take linearly longer, worse on slower devices. | ||
|
|
||
| Benchmarks: | ||
|
|
||
| i. Lab benchmarks: updating a page with 1000 components, static runtime css vs. dynamic runtime css (thanks @mattcosta7 for these!) | ||
|
|
||
| re-rendering components with fixed styles on runtime: 242ms | ||
| re-rendering components with dynamic styles on runtime: 441ms (55% slower) | ||
|
|
||
| ii. Application benchmarks: opting out of IconButton for PR diff view (>500 files, [gh/gh#243269](https://github.com/github/github/pull/243269)) | ||
|
|
||
| dynamic styles with sx prop: 400ms | ||
| dynamic styles with .css files: 165ms (60% faster) | ||
|
|
||
| The other reasons for change are to utilise css variables for theming and improve the guardrails of consuming primer/primitives | ||
|
|
||
| ## Decisions | ||
|
|
||
| 1. Manually refactor styles to css files + css modules | ||
|
Member
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. Would the scope of this ADR include techniques for:
Here are some situations that I was thinking of:
Member
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. Haven't included any of these. Hoping to define those as we go.
Member
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. @siddharthkp these could be worth bringing up at this point as it would influence the technical decision (in this case CSS Modules) as the different approaches have pros/cons when it comes to share and re-use. Or should I read this ADR as more about styling with CSS (as opposed to styled-components) and that decisions like CSS Modules are not a part of the scope of this ADR?
Member
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.
I think css modules of that decision but the implementation details of how do we do that have been left out for future ADRs 😅 |
||
|
|
||
| Use css variables from primer/primitives for inexpensive theming support, especially with server side rendering | ||
|
Member
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. What's the expectation for using CSS Custom Properties along with stuff like breakpoints? From what I remember, syntax like the following would not work: @media (max-width: var(--some-breakpoint)) {
// ...
}Since the
Member
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. We have the option of running our css through postcss and use plugins/postcss-custom-media, I think primer/view_components does the same
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. We ship CSS vars to work with the custom media plugin 🎉
Member
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. @siddharthkp what would the build step for this end up looking like? Would it be at publish/build time for the package or are we expecting end users to include this as part of their build toolchain? @langermank could you share an example of how the CSS Custom Properties from primitives would interop with the custom media plugin?
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 have this running in a remix app with global CSS (not CSS modules). I have a watcher that compiles the PostCSS to CSS. Media query looks like this (and here is what we ship from primitives https://unpkg.com/browse/@primer/[email protected]/tokens-v2-private/css/tokens/functional/size/viewport.css) PostCSS file Built CSS file
Member
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. Thanks @langermank! Appreciate it 🙏 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. If it helps the discussion at all, I'm a big fan of what Bootstrap does with their breakpoints |
||
|
|
||
| 2. Use css modules with hash | ||
|
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. Reading this ADR in isolation, I struggle to see how we've come to this decision. What were the alternatives considered and why do we deem this one the preferred choice?
Member
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. That's fair. I don't think we have considered alternatives to css modules. It was mostly a choice between should we use css modules or not. Are there alternatives that you would suggest should be added here? |
||
|
|
||
| By importing styles in the component, we can create an explicit dependency between the two. This enables applications using primer to have control over bundling and delivery (they can be split by page or bundled as a common chunk for all react pages to share) | ||
|
|
||
| 3. Keep styled-components underneath for supporting sx prop. | ||
|
|
||
| Keep backward compatibility, we might change this in the future | ||
|
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 strongly advocate for dropping support for sx/styled-components
Member
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. I think we don't have an option in the short term in order to not break sx prop in dotcom 😢 After we migrate off sx prop in dotcom, we'll be able to drop styled-components completely! I think I can make that line clearer though - 3. Keep styled-components underneath for supporting sx prop
+ 3. Keep styled-components underneath for supporting sx prop in applications
- Keep backward compatibility, we might change this in the future
+ Maintain backward compatibility for sx in the short term, but do not use it in primer/react |
||
|
|
||
| Code example: | ||
|
|
||
| ```jsx | ||
| import * as css from './ActionList.css' | ||
| import classnames from 'classnames' | ||
joshblack marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| const Button = ({className, sx, ...props}}) => { | ||
| return ( | ||
| <Box | ||
| as="button" | ||
| sx={sx} | ||
| // it's important to pass both internal and props.className to the element | ||
| className={classnames(css.ActionList, className)} | ||
|
||
| {...props} | ||
| > | ||
| </Box> | ||
| ) | ||
| } | ||
| ``` | ||
|
|
||
| ### Out of scope | ||
|
|
||
| Methodologies and guidelines on how to author CSS are not part of this ADR/decision. We will define those as we move components to css and find repeating patterns. | ||
|
|
||
| | ||
|
|
||
| ### Other options considered | ||
|
|
||
| 1. Continue to author styles in typescript, compile them to .css files before shipping | ||
|
|
||
| We benefit from type-safety while authoring components, compiled to .css files with a babel plugin before shipping. | ||
|
|
||
| We are not not chosing this option to have more control on the output by keeping the authored and shipped code similar. | ||
siddharthkp marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
Uh oh!
There was an error while loading. Please reload this page.