facebook / stylex

StyleX is the styling system for ambitious user interfaces.
https://stylexjs.com
MIT License
8.41k stars 310 forks source link

feat: Handle variable fallbacks with firstThatWorks #440

Closed nmn closed 9 months ago

nmn commented 9 months ago

What changed / motivation ?

This PR makes it possible to use stylex.firstThatWorks to also work for fallback values for CSS variables.

Something like this should now work:

margin: stylex.firstThatWorks(vars.foo, 200)

Linked PR/Issues

Fixes #432

Additional Context

A unit test has been added for the new behaviour.


stylex.firstThatWorks is a lightweight transform that simply converts the arguments to an array reverses them. This transform is still unchanged. This is important so that we can convert numbers to the correct type of string.

But after the values are converted to px (or not), it now looks for variables within the array of values and compresses them to handle this case correctly.

It can handle [...values, ...variables, ...values];

Non-contiguous variables are not allowed.

Therefore a complex value like this would work:

height: stylex.firstThatWorks('100dvh', vars.foo, vars.bar, '100vh', 500)

Which will result in the following CSS:

height:var(--foo,var(--bar, 500px));
height:var(--foo,var(--bar, 100vh));
height:100dvh;

The question here is if the first line should just be 500px instead.

IMO, this depends on how CSS handles a CSS variable within an unknown fallback value. If var(--x, bla) is considered a valid style then a change should be made.

github-actions[bot] commented 9 months ago

compressed-size: runtime library

Size change: 0.00 kB Total size: 2.52 kB

View unchanged | Filename: gzip (minify) | kB size | kB change | % change | | :--- | :--- | :--- | :--- | | `./packages/stylex/lib/stylex.js` | **1.04** (3.22) | **0.00** (0.00) | **0.0%** (0.0%) | | `./packages/stylex/lib/StyleXSheet.js` | **1.48** (3.75) | **0.00** (0.00) | **0.0%** (0.0%) |
github-actions[bot] commented 9 months ago

compressed-size: e2e bundles

Size change: 0.00 kB Total size: 1128.55 kB

View unchanged | Filename: gzip (minify) | kB size | kB change | % change | | :--- | :--- | :--- | :--- | | `./apps/rollup-example/.build/bundle.js` | **1005.20** (10185.36) | **0.00** (0.00) | **0.0%** (0.0%) | | `./apps/rollup-example/.build/stylex.css` | **123.34** (773.82) | **0.00** (0.00) | **0.0%** (0.0%) |
nmn commented 9 months ago

Follow up:

height:var(--foo,var(--bar, 500px));
height:var(--foo,var(--bar, 100vh));
height:100dvh;

is equivalent to:

height:var(--foo,var(--bar, 100vh));
height:100dvh;

So if there are multiple fallback values for a CSS variable, only the first value even matters.

As of right now, I don't see a better way to do this. It's possible to use @supports to get around this problem but that would need a bigger architectural change. So, I'm going to merge this as is and add documentation to explain how CSS variables are never considered "invalid", so only a single fallback value would work.