Skip to content
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

Switch most CSS processing to Lightning CSS and upgrade to PostCSS 8 #4645

Closed
wants to merge 13 commits into from

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Jun 7, 2023

This is a reduced/updated version of #3018. It swaps most of our CSS processing to Lightning CSS, which allows us to more easily upgrade to PostCSS 8 (unblocking things like Tailwind examples in the repo).

  • The non-standard nesting syntax (postcss-nested) was replaced with the official CSS nesting syntax. This has one major difference: A nesting selector (&) must be included in nested rules if they start with an element selector (e.g. & div). They are not required in other cases (e.g. .foo), and the spec may later be updated to loosen this requirement further.
  • @inherits, @extend, and @mixin postcss plugins have been replaced with the CSS modules composes property. Rather than duplicating the declarations from the extended rule, this causes both classes to be applied on the element that uses the composed class. Beware that this can cause differences in specificity.
  • The :focus-ring pseudo class has been changed to the now-standard :focus-visible, which is processed by Lightning CSS to be replaced with our .focus-ring class as before.
  • The :hover pseudo class is now always replaced with .is-hovered, whereas before it was only done in release/storybook and not in the docs. Now the docs have a bit of JavaScript that applies the hover class in places that are server rendered. That is done by setting the data-hover attribute to the hover class. This means the docs now works as the rest of RSP in regards to touch devices and hover states.
  • Logical properties are now compiled differently. dir nesting is now possible, and in many cases, we don't even need to compile them since most individual properties are already supported natively except a few (e.g. border radius).

The remaining PostCSS plugins are:

Potentially we can get rid of the rest of these later but it will require more refactoring to our source CSS. For now, getting rid of most of the other plugins made upgrading to PostCSS 8 much easier.

@rspbot
Copy link

rspbot commented Jun 8, 2023

@rspbot
Copy link

rspbot commented Jun 8, 2023

@rspbot
Copy link

rspbot commented Jun 8, 2023

…n custom properties

No idea why support for that was removed in postcss-custom-properties v8
@rspbot
Copy link

rspbot commented Jun 9, 2023

Should not be relying on [dir] selector being added for logic properties
@rspbot
Copy link

rspbot commented Jun 9, 2023

@rspbot
Copy link

rspbot commented Jun 9, 2023

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

@dsmmcken
Copy link
Contributor

  • postcss-custom-properties to inline custom properties defined in :root selectors
  • Our custom postcss-custom-properties-mapping and postcss-custom-properties-passthrough (ie xvar) plugins, which inline spectrum variables with fallback.

Can I ask about these plugins purpose? Isn't custom properties widely supported well before the targeted versions in the browserlist for this project now?

The reason I ask, is because I would like to swap a few variables, like font-family (#2385, #4344) and not inlining variables would make that a lot easier.

@devongovett
Copy link
Member Author

The problem is that Spectrum has something like ~20,000 individual tokens, so we have to inline them to keep bundle size reasonable. Regardless, the variables are considered an implementation detail and are not intended for public use. We can change them at any time, and have in the past, so I wouldn't recommend overriding them.

@dsmmcken
Copy link
Contributor

dsmmcken commented Aug 1, 2023

The problem is that Spectrum has something like ~20,000 individual tokens, so we have to inline them to keep bundle size reasonable.

Thanks for the reply! That does make sense. Though, I imagine they might compress well because of how duplicative they are.

I was curious, so I did some envelop math (that may be wrong) on the gzip size difference and only got an <8kb difference overall. Some files were actually smaller, as repeated inline font declarations for example are actually larger than just using the variable. That might be an area where variable passthrough should be considered, though I didn't quite figure out why the font stuff was getting in each packages main.css file in the first place.

envelop math

# with inlining commented out by removing
# postcss-custom-properties
# postcss-custom-properties-passthrough
# postcss-calc
# postcss-custom-properties-mapping
yarn build
# grab and merge all the main.css packages together for rough estimate
find packages -name main.css -exec cat {} + > not_inlined.css

# with inlining
yarn build
find packages -name main.css -exec cat {} + > inlined.css

gzip -c not_inlined.css > not_inlined.css.gz
gzip -c inlined.css > inlined.css.gz

# size in bytes
du -b not_inlined.css.gz inlined.css.gz
145935  not_inlined.css.gz
138083  inlined.css.gz

We can change them at any time, and have in the past, so I wouldn't recommend overriding them.

I acknowledge that I am not using it as intended, but fonts feel like something that would be reasonable as part of a theme for external use cases for those who want minimal customization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants