Conversation
Add a warning (severity: 'warning') when users use base tokens like --base-text-weight-*, --base-text-size-*, or --base-text-lineHeight-* instead of functional typography tokens. The warning informs users but does not block CI or cause lint errors. Also emits the base token warning after auto-fix replaces a hardcoded value with a base token, so users are aware of the recommendation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
There was a problem hiding this comment.
Pull request overview
This PR adds a warning-severity lint message for base typography token usage (e.g., --base-text-weight-*, --base-text-size-*, --base-text-lineHeight-*) in the primer/typography Stylelint plugin. Users are nudged toward functional tokens for better semantic meaning, while hardcoded values remain errors and base tokens become warnings that won't block CI.
Changes:
- Added a
baseTokenwarning message and base token detection logic in the typography plugin so thatvar(--base-*)typography tokens are reported as warnings rather than silently accepted. - Implemented post-fix warning emission so that when auto-fix replaces a hardcoded value with a base token, an additional
baseTokenwarning is emitted. - Updated test cases to move base token usage from
accepttoreject(with warning) and added test cases for base font-size and line-height.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
plugins/typography.js |
Added baseToken message, base token regex detection after variable validation, and post-fix warning emission for auto-fixed base tokens |
__tests__/typography.js |
Moved base font-weight token from accept to reject with warning assertion; added new reject cases for base font-size and line-height tokens |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (/var\(--base-/.test(value)) { | ||
| report({ | ||
| index: declarationValueIndex(declNode), | ||
| endIndex: declarationValueIndex(declNode) + value.length, | ||
| message: messages.baseToken(value), | ||
| node: declNode, | ||
| result, | ||
| ruleName, | ||
| severity: 'warning', | ||
| }) | ||
| } |
There was a problem hiding this comment.
The __tests__/__fixtures__/good/ fixture files still contain base typography tokens that will now trigger warnings:
example.pcss:19—font-weight: var(--base-text-weight-medium)example.scss:19—font-weight: var(--base-text-weight-semibold)example.scss:23—font-weight: var(--base-text-weight-normal)
While warnings won't cause npm run test:stylelint to exit with a non-zero code, the "good" fixtures are conceptually the reference for clean/passing code. These files should be updated to use functional tokens (e.g., --text-title-weight-medium) or have /* stylelint-disable primer/typography */ comments to suppress the warning, keeping the fixtures truly warning-free.
|
|
||
| if (checkForVariable(validValues, value)) { | ||
| // Warn when using base tokens (e.g. --base-text-weight-*) instead of functional tokens | ||
| if (/var\(--base-/.test(value)) { |
There was a problem hiding this comment.
The regex /var\(--base-/ doesn't account for optional whitespace inside var(). While uncommon, a declaration like font-weight: var( --base-text-weight-semibold ) would pass checkForVariable (matching the variable name) but not trigger the warning (the regex expects var(--base- with no space). Consider using a more lenient pattern such as /var\(\s*--base-/ for robustness.
There was a problem hiding this comment.
While this would make the code stronger, I think we have formatting rules that wouldn't allow you to include whitespace inside the var declaration. I'm fine shipping as is
| if (didFix && isBaseTokenFix) { | ||
| const newValue = declNode.value | ||
| report({ | ||
| index: declarationValueIndex(declNode), | ||
| endIndex: declarationValueIndex(declNode) + newValue.length, | ||
| message: messages.baseToken(newValue), | ||
| node: declNode, | ||
| result, | ||
| ruleName, | ||
| severity: 'warning', | ||
| }) | ||
| } |
There was a problem hiding this comment.
The didFix && isBaseTokenFix code path (lines 182–193) only fires in Stylelint's fix mode, and none of the test cases verify that the baseToken warning is actually emitted after a fix. The existing fixable test cases (e.g., font-weight: 500) only assert the rejected message, which is evaluated in non-fix mode. Consider adding a test that validates the baseToken warning is emitted post-fix — for instance, by using warnings (plural) with two entries for a fixable case, or by adding a dedicated test that runs in fix mode and checks for the warning on the fixed output.
|
|
||
| if (checkForVariable(validValues, value)) { | ||
| // Warn when using base tokens (e.g. --base-text-weight-*) instead of functional tokens | ||
| if (/var\(--base-/.test(value)) { |
There was a problem hiding this comment.
While this would make the code stronger, I think we have formatting rules that wouldn't allow you to include whitespace inside the var declaration. I'm fine shipping as is
Motivation
Base typography tokens (
--base-text-weight-*,--base-text-size-*,--base-text-lineHeight-*) are lower-level primitives. Users should prefer functional tokens (e.g.--text-title-weight-medium) for better semantic meaning and maintainability. Currently, base tokens are silently accepted — this PR surfaces a warning to nudge users toward functional alternatives without blocking their workflow.Approach
The
primer/typographyplugin now detectsvar(--base-*)values and reports them withseverity: 'warning'. This means:--text-title-size-medium) → accepted silently ✅--base-text-weight-semibold) → warningfont-weight: 500) → error ❌ (unchanged)When auto-fix replaces a hardcoded value with a base token (e.g.
500→var(--base-text-weight-medium)), the base token warning is also emitted so the user is aware the replacement is a base-level token.Changes
plugins/typography.js— AddedbaseTokenmessage, base token detection after variable validation, and post-fix warning emission__tests__/typography.js— Moved base token case fromaccepttorejectwith warning, added test cases for base font-size and line-heightCreated with GitHub Desktop