ADR: Styling with css #3080

siddharthkp · 2023-03-27T13:27:06Z

github-actions · 2023-03-27T13:32:10Z

size-limit report 📦

Path	Size
dist/browser.esm.js	121.18 KB (0%)
dist/browser.umd.js	121.47 KB (0%)

contributor-docs/adrs/adr-016-css.md

Co-authored-by: Armağan <broccolinisoup@github.com>

joshblack

Looking great! Just had a couple of questions around scope of the ADR but overall just wanted to say that I'm excited for the direction and really appreciate how much time you spent into preparing this and for documenting and sharing your work along the way 🔥

contributor-docs/adrs/adr-016-css.md

joshblack · 2023-03-29T15:16:43Z

contributor-docs/adrs/adr-016-css.md

+
+## Decisions
+
+1. Manually refactor styles to css files + css modules


Would the scope of this ADR include techniques for:

Sharing between CSS files

Managing dependencies between CSS files

Here are some situations that I was thinking of:

I have a helper class that I want to make sure is available to a component (like sr-only)

I have a helper mixin (in the sass sense) to reset a button in order to customize styling

I have a helper mixin or function to help with managing breakpoints

Haven't included any of these. Hoping to define those as we go.

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.

https://github.com/primer/react/blob/adr-css/contributor-docs/adrs/adr-016-css.md#out-of-scope

@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?

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?

I think css modules of that decision but the implementation details of how do we do that have been left out for future ADRs 😅

joshblack · 2023-03-29T15:18:39Z

contributor-docs/adrs/adr-016-css.md

+
+1. Manually refactor styles to css files + css modules
+
+   Use css variables from primer/primitives for inexpensive theming support, especially with server side rendering


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 @media query is not scoped under a :root. Would we use helper mixins/functions/etc?

We have the option of running our css through postcss and use plugins/postcss-custom-media, I think primer/view_components does the same

We ship CSS vars to work with the custom media plugin 🎉

@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?

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/primitives@7.8.3/tokens-v2-private/css/tokens/functional/size/viewport.css)

@custom-media --md-n-above (width >= 768px);

PostCSS file

@media (--md-n-above) { .AppLayout { --sidepane-width: var(--size-280); --header-height: var(--size-64); --topbar-visibility: visible; } }

Built CSS file

@media (min-width: 768px) { .AppLayout { --sidepane-width: var(--size-280); --header-height: var(--size-64); --topbar-visibility: visible; } }

Thanks @langermank! Appreciate it 🙏

If it helps the discussion at all, I'm a big fan of what Bootstrap does with their breakpoints
https://getbootstrap.com/docs/5.0/layout/breakpoints/

colebemis

Thanks for all the thought you've put into this decision, @siddharthkp! Excited to move forward with this 🎉

keithamus · 2023-03-30T09:23:39Z

contributor-docs/adrs/adr-016-css.md

+
+   Use css variables from primer/primitives for inexpensive theming support, especially with server side rendering
+
+2. Use css modules with hash


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?

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?

mperrotti · 2023-03-31T19:23:06Z

contributor-docs/adrs/adr-016-css.md

+
+3. Keep styled-components underneath for supporting sx prop.
+
+   Keep backward compatibility, we might change this in the future


I strongly advocate for dropping support for sx/styled-components

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

joshblack · 2023-04-04T16:23:09Z

Quick question, is there a prototype that demonstrates what the package input and output would look like after accepting this ADR? In particular, would be curious about the build step and what files end up getting published and what a consumer build setup would look like.

Was trying to put something together while reading through the ADR and I was struggling with how to:

Emit JavaScript files
Make sure the JavaScript files include the import to the CSS Module (or should this be to a CSS file?)
Find a way to process the CSS Module through something like Postcss without removing the import

langermank · 2023-04-04T20:13:56Z

contributor-docs/adrs/adr-016-css.md

+      as="button"
+      sx={sx}
+      // it's important to pass both internal and props.className to the element
+      className={classnames(css.ActionList, className)}


Im not sure if this is out of scope, but we might want to document this decision to offer className as a prop instead of sx moving forward

We'd need to keep supporting both sx and className for the short term in order to not break dotcom. But, after we migrate off sx prop in dotcom, we'll be able to drop sx and styled-components completely!

siddharthkp · 2023-04-12T13:13:22Z

@joshblack, yep! I have a spike here with 2 possible options in the description: #3065

tl;dr: there's are rollup plugins we can use:

single css file: https://github.com/egoist/rollup-plugin-postcss
css file per component: https://github.com/DanielAmenou/rollup-plugin-lib-style

We'd probably start with option 1 and shift to option 2, when needed in the future

joshblack · 2023-04-12T16:58:22Z

@siddharthkp thanks so much! Taking a look 👀

For the single CSS file approach, would there be a concern about bundle size? Specifically if you're only using a handful of components but you're bringing in the styles for all of them. Or are things implemented in dotcom such that this file would be shared across services?

Another question (sorry for going on and on lol), how would we handle things like "resets" or "globals" that we want to share or dedupe? For example, if we had a common reset or common CSS like from primitives for setting themes.

siddharthkp · 2023-04-14T14:02:58Z

For the single CSS file approach, would there be a concern about bundle size? Specifically if you're only using a handful of components but you're bringing in the styles for all of them. Or are things implemented in dotcom such that this file would be shared across services?

I think it definitely could be a concern once we reach the stage where components that are not common across pages take up a significant % in the shared bundle. That's the point where splitting a big cached bundle would be useful. (we'd still have to test that with dotcom though, because there are a bunch of big files that get downloaded so splitting might just make it slower 😅)

But from the library's side, we would shift from shipping a single bundle to one global file + multiple component files which can be bundled by the application's config based on component usage across pages.

I think we should start with a single bundle and test splitting it once it has a good amount of components (10-15)

Another question (sorry for going on and on lol)

nah nah, keep em coming please!

how would we handle things like "resets" or "globals" that we want to share or dedupe? For example, if we had a common reset or common CSS like from primitives for setting themes

(will need to test more when we get to that stage) I think even when we ship one file per component, we will have a global file that needs to loaded on all pages. I don't have a clear approach on how we would author or generate that shared file, leaving that for a future ADR that comes from usage.

joshblack · 2023-04-17T18:25:33Z

I think we should start with a single bundle and test splitting it once it has a good amount of components (10-15)

From the consumer side, would this kind of look like this in the beginning:

// Some entrypoint file
import '@primer/react/css/styles.css';

// Use components like always
import { Button } from '@primer/react';

And then if we want to split things by-component:

// Some entrypoint file

// Base styles
import '@primer/react/css/base.css';

// Component styles
import '@primer/react/css/button.css';

// Use components like always
import { Button } from '@primer/react';

Or would it be that importing Button automatically bring in the css file?

I think even when we ship one file per component, we will have a global file that needs to loaded on all pages.

Agreed, generally I think the "shared" modules end up being:

Reset styles
Base styles, e.g. for fonts or motion or for CSS Custom Properties on :root

This kind of implicit dependency/ordering seems hard to express without some kind of module system which is why this was coming up in case we need some kind of processing on top of just plain css files (e.g. if we want to use imports from postcss or sass modules)

broccolinisoup

This is looking amazing 🔥 Thank you so much for all the effort you put into this work, it is appreciated so much 💖 I am very excited for the direction we are going to 🚀

github-actions · 2023-07-01T09:04:30Z

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

lesliecdubs · 2023-07-03T20:02:50Z

Bump @siddharthkp as this is going stale. Let me know if this ADR warrants an assigned decision-maker; otherwise, let's work on resolving the open comments and moving toward merge.

siddharthkp · 2023-07-04T10:04:02Z

Yep, it's on my todo list!

siddharthkp added 2 commits March 20, 2023 15:12

wup

341408d

fill in draft

bf9a46e

siddharthkp requested review from a team and JoshBowdenConcepts March 27, 2023 13:27

This comment was marked as off-topic.

Sign in to view

siddharthkp self-assigned this Mar 27, 2023

siddharthkp removed the request for review from JoshBowdenConcepts March 27, 2023 13:27

siddharthkp added the skip changeset This change does not need a changelog label Mar 27, 2023

siddharthkp temporarily deployed to github-pages March 27, 2023 13:36 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3080 March 27, 2023 13:36 Inactive

broccolinisoup added the ADR label Mar 27, 2023

broccolinisoup reviewed Mar 29, 2023

View reviewed changes

contributor-docs/adrs/adr-016-css.md Outdated Show resolved Hide resolved

broccolinisoup reviewed Mar 29, 2023

View reviewed changes

contributor-docs/adrs/adr-016-css.md Outdated Show resolved Hide resolved

Apply suggestions from code review

321e425

Co-authored-by: Armağan <broccolinisoup@github.com>

siddharthkp temporarily deployed to github-pages March 29, 2023 15:21 — with GitHub Actions Inactive

joshblack reviewed Mar 29, 2023

View reviewed changes

github-actions bot temporarily deployed to storybook-preview-3080 March 29, 2023 16:28 Inactive

colebemis approved these changes Mar 29, 2023

View reviewed changes

keithamus reviewed Mar 30, 2023

View reviewed changes

mperrotti approved these changes Mar 31, 2023

View reviewed changes

langermank reviewed Apr 4, 2023

View reviewed changes

use clsx instead of classnames

15bc10e

siddharthkp temporarily deployed to github-pages April 12, 2023 14:18 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3080 April 12, 2023 14:18 Inactive

Update ADR based on feedback, add issue link

8769686

siddharthkp deployed to github-pages April 26, 2023 12:40 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-3080 April 26, 2023 12:40 View deployment

joshblack approved these changes Apr 26, 2023

View reviewed changes

broccolinisoup approved these changes May 2, 2023

View reviewed changes

github-actions bot added the Stale label Jul 1, 2023

github-actions bot removed the Stale label Jul 3, 2023

siddharthkp added this pull request to the merge queue Jul 4, 2023

Merged via the queue into main with commit 0eb52a4 Jul 4, 2023

siddharthkp deleted the adr-css branch July 4, 2023 13:14


		## Decisions

		1. Manually refactor styles to css files + css modules


		1. Manually refactor styles to css files + css modules

		Use css variables from primer/primitives for inexpensive theming support, especially with server side rendering


		Use css variables from primer/primitives for inexpensive theming support, especially with server side rendering

		2. Use css modules with hash


		3. Keep styled-components underneath for supporting sx prop.

		Keep backward compatibility, we might change this in the future

ADR: Styling with css #3080

ADR: Styling with css #3080

Conversation

siddharthkp commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

github-actions bot commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Uh oh!

Uh oh!

joshblack left a comment

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

Out of scope

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthkp Apr 6, 2023 • 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!

siddharthkp Mar 29, 2023 • 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!

langermank Apr 4, 2023 • 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!

colebemis left a comment

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!

Choose a reason for hiding this comment

Uh oh!

joshblack commented Apr 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthkp commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshblack commented Apr 12, 2023

Uh oh!

siddharthkp commented Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshblack commented Apr 17, 2023

Uh oh!

broccolinisoup left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 1, 2023

Uh oh!

lesliecdubs commented Jul 3, 2023

Uh oh!

siddharthkp commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

siddharthkp Apr 6, 2023 •

edited

Loading

siddharthkp Mar 29, 2023 •

edited

Loading

langermank Apr 4, 2023 •

edited

Loading

siddharthkp commented Apr 12, 2023 •

edited

Loading

siddharthkp commented Apr 14, 2023 •

edited

Loading