Skip to content

refactor: Generate stable style macro class names between versions #8118

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

Merged
merged 3 commits into from
Apr 28, 2025

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Apr 18, 2025

This refactors the style macro so that the CSS class names it generates are stable between versions. This fixes issues that occurred when multiple versions were on the page simultaneously.

To implement this, there are now two steps:

  1. Convert theme-specific properties to native CSS properties. This can sometimes result in a single property expanding into multiple.
  2. Convert CSS properties to rules.

The selector for each rule is generated based on the native CSS property name, CSS condition (e.g. media query), and CSS property value. By default, these are hashed. However, certain properties/values have shorter pre-computed names. For example, fontSize maps to a and the max-content value of the maxHeight property maps to z. These short values were determined by calculating the frequency of use in our CSS, so that the most frequently used values have the shortest names. These are stored in a JSON file, and should never be edited.

In the future, when new values are added or modified they will by default be hashed. If we want to add additional short values, they can be appended, but an existing name can never be re-used. For example, if a color token was updated to a new value, we'd generate a new name for the new value and leave the old one as it was before.

@@ -564,10 +564,7 @@ const sortIcon = style({
default: 8,
isButton: 'text-to-visual'
},
verticalAlign: {
default: 'bottom',
isButton: 0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not a valid value for this property and it seems to have no effect.

"adobe-clean-serif, \"Source Serif\", Georgia, serif": "i",
"source-code-pro, \"Source Code Pro\", Monaco, monospace": "j"
},
"backgroundColor": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Color properties currently do not include all possible values defined in the theme, because there are a lot of them. Right now we only include values currently being used in S2 components. If applications use additional colors then those will currently be hashed. We can additional shorter color mappings in the future but I want to be sure that things have stabilized before assigning these names, since these short names cannot be reused in the future.

@rspbot
Copy link

rspbot commented Apr 18, 2025

@rspbot
Copy link

rspbot commented Apr 19, 2025

'gridRowStart',
'gridRowEnd',
'gridColumn',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These properties don't actually exist in our theme

@@ -349,8 +349,8 @@ let gridTrackSize = (value: GridTrackSize) => {
const transitionProperty = {
// var(--gp) is generated by the backgroundImage property when setting a gradient.
// It includes a list of all of the custom properties used for each color stop.
default: 'color, background-color, var(--gp), border-color, text-decoration-color, fill, stroke, opacity, box-shadow, transform, translate, scale, rotate, filter, backdrop-filter',
colors: 'color, background-color, var(--gp), border-color, text-decoration-color, fill, stroke',
default: 'color, background-color, var(--gp, color), border-color, text-decoration-color, fill, stroke, opacity, box-shadow, transform, translate, scale, rotate, filter, backdrop-filter',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is --gp still correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the missing fallback was a bug before. when --gp is not defined, this resulted in all properties transitioning instead of only the listed ones. Now I made it fall back to something already in the list.

@devongovett
Copy link
Member Author

Additional theme changes are coming in subsequent PRs.

@rspbot
Copy link

rspbot commented Apr 28, 2025

@devongovett devongovett added this pull request to the merge queue Apr 28, 2025
Merged via the queue into main with commit 1dd65ff Apr 28, 2025
30 checks passed
@devongovett devongovett deleted the style-macro-stable-classes branch April 28, 2025 17:54
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.

5 participants