WIP: refactor(styles): add selector information

[levithomason]

😕 Problems

1. Style files are currently hard to read and manage due to their complexity. The complexity is caused by heavily nested conditionals and the use of abstractions (like helper functions).

2. Style files currently cannot be used outside of React. This is due to coupling styles to runtime only information, i.e. props.

3. Styles cannot be used in a build step to generate flat style sheets. See the findings in the generate-css branch. This is mainly due to the lack of selector information. There is no way to know what props a style file supports, so no automated invocations can be made.

4. Perf is a problem. Styles are computed for each part of each component, including duplicate calculations when there are multiple instances of the same component on the page.

😄 Solution

This PR is exploring a new approach to writing style files. Both statusStyles.ts and avatarStyles.ts have been updated.

Salient points:

1. Styles include selector info and order of precedence (enables build time compilation).
2. Runtime props are not used. Instead, they are encoded into prop matcher objects.
3. Style files are smaller and easier to read and maintain.
4. Styles can be calculated once in the Provider for all components in the app.

---

Other Findings

Style objects cannot rely on computed variables

Reference statusStyles on master:
https://github.com/stardust-ui/react/blob/a6c4ed73beed835b1c726f506068cf19a0ecbe5b/packages/react/src/themes/teams/components/Status/statusStyles.ts#L67

The border width and border style are guarded by a conditional on v.borderColor. However, the border color varible can only be passed at runtime through the variables prop. This is done by the Avatar. This means, unless you use React and runtime variable overrides through props (or .create()) there is no way to invoke the border styles.

Clients using other frameworks are not able to use the style. Also, flat style sheets cannot be computed for this style. There is no way to invoke the border color override using these approaches.

Anti-pattern of passing CSS through props is no longer possible

This is a good thing. However, places where the color prop is currently passing CSS values which are used directly in styles is no longer possible. Styles do not accept props, therefore, prop values are not available.

[levithomason]

The fix option doesn't work with a monorepo... we still get nice errors if there is something wrong. The dev can just run yarn in that case.

[levithomason]

The status itself no longer needs these variables. These are only used in the context of the Avatar. So, the Avatar adds styles to the Status component in this case.

[levithomason]

Using a boxShadow to fake a border removes the need for fancy content + border width calculations.

[levithomason]

Styles are now a single function that returns an object with styles keyed by component part. Opposed to each part being its own function.

[codecov]

Codecov Report

Merging #1301 into master will increase coverage by 0.22%.
The diff coverage is 35.48%.

@@            Coverage Diff             @@
##           master    #1301      +/-   ##
==========================================
+ Coverage   73.25%   73.48%   +0.22%     
==========================================
  Files         825      823       -2     
  Lines        6207     6177      -30     
  Branches     1796     1791       -5     
==========================================
- Hits         4547     4539       -8     
+ Misses       1655     1633      -22     
  Partials        5        5

Impacted Files	Coverage Δ
.../themes/teams/components/Avatar/avatarVariables.ts	0% <ø> (ø)	⬆️
...high-contrast/components/Avatar/avatarVariables.ts	0% <ø> (ø)	⬆️
packages/react/src/components/Avatar/Avatar.tsx	100% <ø> (ø)	⬆️
.../themes/teams/components/Status/statusVariables.ts	0% <0%> (ø)	⬆️
...src/themes/teams/components/Status/statusStyles.ts	100% <100%> (+86.48%)	⬆️
...src/themes/teams/components/Avatar/avatarStyles.ts	100% <100%> (+80%)	⬆️
packages/react/src/lib/resolveComponentRules.ts	32.14% <32.14%> (ø)
...ages/react/test/specs/behaviors/testDefinitions.ts	90.78% <0%> (-0.35%)	⬇️
.../src/themes/base/components/Loader/loaderStyles.ts	12.5% <0%> (ø)	⬆️
packages/react/src/lib/shouldHandleOnKeys.ts	100% <0%> (ø)	⬆️
... and 18 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58e0d50...fb864a9. Read the comment docs.

[levithomason]

Component parts are now arrays of tuples. The tuples are [selector, style]. If the props object matches the component's props, then the style is applied. Matched styles are deep merged from top to bottom.

This makes this syntax work much like a CSS sheet:

    [{ color: 'red' }, { backgroundColor: v.backgroundRed }],
    [{ color: 'orange' }, { backgroundColor: v.backgroundOrange }],

Which is equivalent to this SCSS:

    .ui-status.red { background-color: $backgroundRed; }
    .ui-status.orange { background-color: $backgroundOrange; }

The selector information will allow us to transform props into CSS selectors as well.

[levithomason]

Component parts now define an array of [selector, style] tuples. When the selector object matches the props of the component, then the style is applied.

A null selector matches any props object so the styles are always applied.

const styles = {
  root: [
    [null, [{ display: 'block' }],
    [{ size: 'small', fontSize: '0.9em' }],
  ],
}

Here are the styles that would be produced:

<Status />
// => { display: 'block' }

<Status size='small' foo />
// => { display: 'block', fontSize: '0.9em' }

The foo prop above does not prevent the match from being made. The Status still has a size prop of small, so it matches the second root style.

[levithomason]

backportComponentStyle is a helper that transforms the new proposed syntax into the previous syntax. This just means we don't need to update the rest of our code during a migration.

If we did ship this in transition, we'd want to use this utility anywhere we have incoming styles from users.

---

We also removed above 20 lines from this file :D Try looking at the rendered version next to the original. 🎉

[levithomason]

These used to be computed in the style function, based on the size prop. Now that props aren't exposed in the style functions, we should pre-calculate all values and hoist them to variables.

Styles only receive variables at this point.

[kuzhelov]

awesome - curious to test this approach against more "involved" styles, like the ones of MenuItem in Teams theme. However, in either case, semantics of this selector-based approach, essentially, reflects semantics of platform-agnostic styles, so this is what we should strive for.

Let me try the idea of this approach for MenuItem styles, just to understand if there are other problems, apart from the ones you've already pointed out and which, in fact, should be fixed (like the case of styles dependent on variable's presense)

[levithomason]

Indeed. @layershifter and I paired for a couple hours on this. There are many things to learn by converting some styles over. Things like:

• Using variables or logic in selectors is not possible
• Calculations in style objects should be avoided and hoisted to variables files
• Passing prop values directly to style objects (like color) is not possible
• Some calculations need to be replaced with CSS calc()
• We should be careful about defining variables with pixel number values. We do this when we use pxToRem() in styles on the variable. This means the user can't pass a different unit in their variables since it will always be converted to rem.
• ...etc

These limitations are made clear when refactoring a style function. It is also very obvious that these are good things when you think through them. It points out many issues we currently have in styles.

I will finish the avatar styles now...

[levithomason]

Done with avatarStyles now, see the diff here and the finished file here.

Problem #1 has been solved. The next step here should be to use these to generate some static styles to prove out solutions to problems #2 and #3.

Finally, @kuzhelov has been working on solutions to a separate list of issues related to styles. We should merge the solutions here with those solutions to ensure we're covering all of the bases.

[DustyTheBot]

	Warnings
⚠️	There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.
⚠️	New package.json added: packages/style/package.json. Make sure you have approval before merging!

Generated by 🚫 dangerJS

[mnajdova]

I've replaced color: 'green' with the identical styles: {backgroundColor: 'green'} for avoiding screener regressions. Will replace this with state: 'success' in a separate PR, where the changes would be scoped and easily tracked.

[layershifter]

No success, closing for now.