WCSS Change Request Logic Hardening#281
Merged
jordanjones243 merged 3 commits intoMay 1, 2026
Merged
Conversation
…rameter Surface accent letter-spacing intent at the theme call site instead of inferring it from the accent-config 2xl entry. The inference was brittle: a future theme defining letter-spacing on some sizes but not 2xl would have been miscategorized for the entire accent group.
The per-size guard on var emission and the theme-level $use-letter-spacing flag on class emission were two independent gates that could disagree. A mixed config could produce a class referencing an undefined var, causing letter-spacing to resolve to unset rather than inherit. Make the theme flag the single source of truth.
…cope The mixin name suggests it covers all four type categories, but the $use-letter-spacing parameter only ever flowed to generate-accent-classes. Rename to $accent-use-letter-spacing so the scope is visible at the call site without stepping into the mixin body.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the typography theme generation SCSS mixins to take an explicit accent letter-spacing flag, consistently propagate it through type class generation, and always emit letter-spacing CSS variables, while disabling accent letter-spacing for the Hawaiian theme. Class diagram for updated SCSS type mixinsclassDiagram
direction LR
class generate_theme {
+mixin generate_theme($theme-name, $body-config, $display-config, $heading-config, $accent-config, $accent-uses-letter-spacing)
}
class generate_theme_type_css_vars {
+mixin generate_theme_type_css_vars($theme-configs)
}
class generate_theme_type_classes {
+mixin generate_theme_type_classes($use-fallback, $fallback-configs, $accent-use-letter-spacing)
}
class generate_body_classes {
+mixin generate_body_classes($use-fallback, $fallback-config)
}
class generate_display_classes {
+mixin generate_display_classes($use-fallback, $fallback-config)
}
class generate_heading_classes {
+mixin generate_heading_classes($use-fallback, $fallback-config)
}
class generate_accent_classes {
+mixin generate_accent_classes($use-fallback, $fallback-config, $accent-use-letter-spacing)
}
class fluid_type_vars {
+mixin generate_fluid_type_vars($css-ns, $type, $size, $theme-config)
--type-family
--type-family-fallback
--type-letter-spacing
--type-weight
--type-line-height
}
generate_theme --> generate_theme_type_css_vars : passes $theme-configs
generate_theme --> generate_theme_type_classes : passes $accent-uses-letter-spacing as $accent-use-letter-spacing
generate_theme_type_classes --> generate_body_classes : body fallback config
generate_theme_type_classes --> generate_display_classes : display fallback config
generate_theme_type_classes --> generate_heading_classes : heading fallback config
generate_theme_type_classes --> generate_accent_classes : accent fallback + $accent-use-letter-spacing
fluid_type_vars ..> generate_theme_type_css_vars : uses type-property-check values as CSS vars
Flow diagram for accent letter-spacing flag propagationflowchart LR
subgraph ThemeConfigInputs
TName[$theme-name]
Body[$body-config]
Display[$display-config]
Heading[$heading-config]
Accent[$accent-config]
AccentFlag[$accent-uses-letter-spacing]
end
TName --> GenTheme
Body --> GenTheme
Display --> GenTheme
Heading --> GenTheme
Accent --> GenTheme
AccentFlag --> GenTheme
GenTheme[generate-theme mixin] --> ThemeConfigs[(theme-configs map)]
GenTheme --> GenTypeCssVars[generate-theme-type-css-vars]
GenTheme --> GenTypeClasses[generate-theme-type-classes]
ThemeConfigs --> GenTypeCssVars
GenTypeCssVars --> FluidVars[CSS custom properties
--type-family
--type-family-fallback
--type-letter-spacing
--type-weight
--type-line-height]
GenTypeClasses --> BodyClasses[body classes]
GenTypeClasses --> DisplayClasses[display classes]
GenTypeClasses --> HeadingClasses[heading classes]
GenTypeClasses --> AccentClasses[accent classes]
AccentFlag --> GenTypeClasses
GenTypeClasses --> AccentClasses
subgraph HawaiianThemeOverride
HawaiianTheme[hawaian/index.scss generate-theme call]
HawaiianAccentFlag[$accent-uses-letter-spacing: false]
end
HawaiianTheme --> HawaiianAccentFlag
HawaiianAccentFlag --> GenTheme
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The change in
_fluid-type.scssto always set--*-letter-spacingremoves the null guard; iftype-property-checkcan returnnull, this will emit an invalid CSS variable value, so consider keeping a conditional or providing a safe default. - The new parameter naming around letter-spacing (
$accent-uses-letter-spacingingenerate-themevs$accent-use-letter-spacingingenerate-theme-type-classes) is very similar but not identical; aligning these names would reduce the likelihood of future misconfiguration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change in `_fluid-type.scss` to always set `--*-letter-spacing` removes the null guard; if `type-property-check` can return `null`, this will emit an invalid CSS variable value, so consider keeping a conditional or providing a safe default.
- The new parameter naming around letter-spacing (`$accent-uses-letter-spacing` in `generate-theme` vs `$accent-use-letter-spacing` in `generate-theme-type-classes`) is very similar but not identical; aligning these names would reduce the likelihood of future misconfiguration.
## Individual Comments
### Comment 1
<location path="src/type/mixins/_fluid-type.scss" line_range="12" />
<code_context>
// Set base properties
--#{$css-ns}-#{$type}-#{$size}-family: #{type-property-check($theme-config, $size, 'family')};
--#{$css-ns}-#{$type}-#{$size}-family-fallback: #{type-property-check($theme-config, $size, 'familyFallback')};
+ --#{$css-ns}-#{$type}-#{$size}-letter-spacing: #{type-property-check($theme-config, $size, 'letter-spacing')};
--#{$css-ns}-#{$type}-#{$size}-weight: #{type-property-check($theme-config, $size, 'weight')};
--#{$css-ns}-#{$type}-#{$size}-line-height: #{type-property-check($theme-config, $size, 'line-height')};
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditionally setting the letter-spacing CSS variable may produce empty values when the config is missing that property.
Previously this variable was only emitted when `letter-spacing` existed in the config. Now, when `type-property-check` returns `null`, `#{null}` becomes an empty string, yielding:
```css
--foo-bar-letter-spacing: ;
```
This can change how other code distinguishes between “not set” and “intentionally empty” (e.g., via `var(--..., <fallback>)` or computed style checks).
If the variable should only exist when configured, keep the conditional:
```scss
$letter-spacing: type-property-check($theme-config, $size, 'letter-spacing');
@if $letter-spacing != null {
--#{$css-ns}-#{$type}-#{$size}-letter-spacing: #{$letter-spacing};
}
```
If it must always exist, consider a clear sentinel value (e.g., `initial` or `normal`) instead of an empty value, depending on consumers’ expectations.
</issue_to_address>
### Comment 2
<location path="src/type/mixins/_type-generator.scss" line_range="17-21" />
<code_context>
// Generate theme type classes
-@mixin generate-theme-type-classes($use-fallback: false, $fallback-configs: null, $use-letter-spacing: true) {
+@mixin generate-theme-type-classes($use-fallback: false, $fallback-configs: null, $accent-use-letter-spacing: true) {
@include generate-body-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'body'), null));
@include generate-display-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'display'), null));
@include generate-heading-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'heading'), null));
- @include generate-accent-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'accent'), null), $use-letter-spacing);
+ @include generate-accent-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'accent'), null), $accent-use-letter-spacing);
}
</code_context>
<issue_to_address>
**nitpick:** Align naming between `$accent-use-letter-spacing` and `$accent-uses-letter-spacing` to reduce confusion.
`generate-theme` uses `$accent-uses-letter-spacing` while this mixin uses `$accent-use-letter-spacing`. This small difference (`use` vs `uses`) can cause mistakes with named arguments and code search. Please standardize on one name across both mixins (e.g., `$accent-use-letter-spacing`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Set base properties | ||
| --#{$css-ns}-#{$type}-#{$size}-family: #{type-property-check($theme-config, $size, 'family')}; | ||
| --#{$css-ns}-#{$type}-#{$size}-family-fallback: #{type-property-check($theme-config, $size, 'familyFallback')}; | ||
| --#{$css-ns}-#{$type}-#{$size}-letter-spacing: #{type-property-check($theme-config, $size, 'letter-spacing')}; |
There was a problem hiding this comment.
issue (bug_risk): Unconditionally setting the letter-spacing CSS variable may produce empty values when the config is missing that property.
Previously this variable was only emitted when letter-spacing existed in the config. Now, when type-property-check returns null, #{null} becomes an empty string, yielding:
--foo-bar-letter-spacing: ;This can change how other code distinguishes between “not set” and “intentionally empty” (e.g., via var(--..., <fallback>) or computed style checks).
If the variable should only exist when configured, keep the conditional:
$letter-spacing: type-property-check($theme-config, $size, 'letter-spacing');
@if $letter-spacing != null {
--#{$css-ns}-#{$type}-#{$size}-letter-spacing: #{$letter-spacing};
}If it must always exist, consider a clear sentinel value (e.g., initial or normal) instead of an empty value, depending on consumers’ expectations.
Comment on lines
+17
to
+21
| @mixin generate-theme-type-classes($use-fallback: false, $fallback-configs: null, $accent-use-letter-spacing: true) { | ||
| @include generate-body-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'body'), null)); | ||
| @include generate-display-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'display'), null)); | ||
| @include generate-heading-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'heading'), null)); | ||
| @include generate-accent-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'accent'), null), $use-letter-spacing); | ||
| @include generate-accent-classes($use-fallback, if($fallback-configs != null, map-get($fallback-configs, 'accent'), null), $accent-use-letter-spacing); |
There was a problem hiding this comment.
nitpick: Align naming between $accent-use-letter-spacing and $accent-uses-letter-spacing to reduce confusion.
generate-theme uses $accent-uses-letter-spacing while this mixin uses $accent-use-letter-spacing. This small difference (use vs uses) can cause mistakes with named arguments and code search. Please standardize on one name across both mixins (e.g., $accent-use-letter-spacing).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alaska Airlines Pull Request
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Resolves: # (issue, if applicable)
Summary:
Please summarize the scope of the changes you have submitted, what the intent of the work is and anything that describes the before/after state of the project.
Type of change:
Please delete options that are not relevant.
Checklist:
By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Pull Requests will be evaluated by their quality of update and whether it is consistent with the goals and values of this project. Any submission is to be considered a conversation between the submitter and the maintainers of this project and may require changes to your submission.
Thank you for your submission!
-- Auro Design System Team
Summary by Sourcery
Make type theme generation more explicit and robust around accent letter-spacing behavior.
Bug Fixes:
Enhancements: