chore: Add reference tokens and replace legacy color tokens in style dictionary #3952
Conversation
jkuelz
requested review from
SpyZzey and
just-boris
and removed request for
a team
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3952 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 854 854
Lines 24985 24985
Branches 8802 8802
=======================================
Hits 24278 24278
Misses 659 659
Partials 48 48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
just-boris
requested review from
a team,
NathanZlion,
just-boris and
patrixd
and removed request for
a team,
NathanZlion,
SpyZzey,
just-boris and
patrixd
just-boris
left a comment
just-boris
left a comment
There was a problem hiding this comment.
Approved, the suggestions about eslint rule can be a follow up
| module.exports.rules = { | ||
| 'ban-files': require('./ban-files'), | ||
| 'prefer-live-region': require('./prefer-live-region'), | ||
| 'no-legacy-tokens': require('./no-legacy-tokens'), |
There was a problem hiding this comment.
Good idea to add a linter rule, but could you add it via a PR to another repository: https://github.com/cloudscape-design/build-tools/tree/main/src/eslint
And while adding this, it also needs some tests for positive and negative matches. There are some examples already
| const matches = node.value.match(LEGACY_TOKEN_PATTERN); | ||
| if (matches) { | ||
| matches.forEach(token => { | ||
| const colorFamily = token.match(/color(Grey|Blue|Green|Red|Yellow)/)?.[1]; | ||
| const suggestion = REFERENCE_TOKEN_SUGGESTIONS[`color${colorFamily}`] || 'semantic reference tokens'; | ||
|
|
||
| context.report({ | ||
| node, | ||
| messageId: 'no-legacy-tokens', | ||
| data: { token, suggestion }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This code is repeated two times and can be reused, right?
NathanZlion
left a comment
NathanZlion
left a comment
There was a problem hiding this comment.
Other than the reviews that @just-boris left, LGTM.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Description
This change introduces reference tokens for primary, neutral, and status colors into our design token system. This is one of a series of updates to implement better out of the box global theming. We are keeping the legacy tokens for now so that we don't break our internal package builds which still rely on them in the meantime. Because we are keeping both tokens, this update also adds an eslint rule to tell developers to use the new tokens if the old ones are referenced.
Related links, issue #, if available: [quip: ac9YAbqsd7e6], CR-229486129
How has this been tested?
Ran through dev pipeline: AwsUi-v3-jkuelz
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.