-
Notifications
You must be signed in to change notification settings - Fork 18
Warn on base typography token usage #677
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ export const messages = ruleMessages(ruleName, { | |
| // one possible replacement | ||
| return `Please replace '${value}' with Primer typography variable '${replacement['name']}'. https://primer.style/foundations/primitives/typography` | ||
| }, | ||
| baseToken: value => | ||
| `'${value}' is a base token. Consider using a functional Primer typography variable instead. https://primer.style/foundations/primitives/typography`, | ||
| }) | ||
|
|
||
| const fontWeightKeywordMap = { | ||
|
|
@@ -118,6 +120,18 @@ const ruleFunction = primary => { | |
| } | ||
|
|
||
| if (checkForVariable(validValues, value)) { | ||
| // Warn when using base tokens (e.g. --base-text-weight-*) instead of functional tokens | ||
| if (/var\(--base-/.test(value)) { | ||
| report({ | ||
| index: declarationValueIndex(declNode), | ||
| endIndex: declarationValueIndex(declNode) + value.length, | ||
| message: messages.baseToken(value), | ||
| node: declNode, | ||
| result, | ||
| ruleName, | ||
| severity: 'warning', | ||
| }) | ||
| } | ||
|
Comment on lines
+124
to
+134
|
||
| return | ||
| } | ||
|
|
||
|
|
@@ -144,10 +158,13 @@ const ruleFunction = primary => { | |
| } | ||
| const replacement = getReplacements() | ||
| const fixable = replacement && !replacement.length | ||
| const isBaseTokenFix = fixable && replacement['name'].startsWith('--base-') | ||
| let didFix = false | ||
| let fix = undefined | ||
| if (fixable) { | ||
| fix = () => { | ||
| declNode.value = value.replace(value, `var(${replacement['name']})`) | ||
| didFix = true | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -160,6 +177,20 @@ const ruleFunction = primary => { | |
| ruleName, | ||
| fix, | ||
| }) | ||
|
|
||
| // When auto-fix replaces a value with a base token, also report the base token warning | ||
| 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', | ||
| }) | ||
| } | ||
|
Comment on lines
+182
to
+193
|
||
| }) | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex
/var\(--base-/doesn't account for optional whitespace insidevar(). While uncommon, a declaration likefont-weight: var( --base-text-weight-semibold )would passcheckForVariable(matching the variable name) but not trigger the warning (the regex expectsvar(--base-with no space). Consider using a more lenient pattern such as/var\(\s*--base-/for robustness.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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