-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat: registers sd-transforms; includes refs in source-tokens-only output; resolves refs warnings #3219
feat: registers sd-transforms; includes refs in source-tokens-only output; resolves refs warnings #3219
Conversation
a68f6df
to
e8d310f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## Peter_Kulko/style-dictionary-v4 #3219 +/- ##
================================================================
Coverage 93.76% 93.76%
================================================================
Files 229 229
Lines 3784 3784
Branches 879 906 +27
================================================================
Hits 3548 3548
+ Misses 232 229 -3
- Partials 4 7 +3 ☔ View full report in Codecov by Sentry. |
5289111
to
97c585b
Compare
@@ -82,7 +84,6 @@ async function buildTokensCommand(commandArgs) { | |||
}, | |||
}, | |||
], | |||
transforms: StyleDictionary.hooks.transformGroups.css.filter(item => item !== 'size/rem').concat('color/sass-color-functions', 'str-replace'), |
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.
[inform] abstracted into transformGroup: 'paragon-css'
, although the default CSS transform size/rem
is now included, instead of filtered like here.
It seems the primary reason it was filtered out was due to tokens that defined a number while having a $type: "dimension"
instead of $type: "number"
. Ensuring the number type keeps the number value with appending rem
as expected, e.g.
--pgn-spacing-card-columns-count: 3;
@@ -36,26 +36,26 @@ | |||
--pgn-transition-base: all .2s ease-in-out; /* Generic transition for any property change */ | |||
--pgn-transition-progress-bar-bar-transition: width .6s ease; | |||
--pgn-transition-progress-bar-bar-animation-timing: 1s linear infinite; | |||
--pgn-transition-form-control: background-color .15s ease-in-out, border-color .15s ease-in-out, box-shadow .15s ease-in-out; | |||
--pgn-transition-form-input: border-color .15s ease-in-out, box-shadow .15s ease-in-out; | |||
--pgn-transition-form-control: background-color 0.15s ease-in-out, border-color 0.15s ease-in-out, box-shadow 0.15s ease-in-out; |
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.
[inform] sd-transforms
includes a transform to add leading zeros where previously missing.
--pgn-typography-line-height-sm: 1.5; /* Small line height. */ | ||
--pgn-typography-line-height-lg: 1.5; /* Large line height. */ | ||
--pgn-typography-line-height-base: 1.5556; /* Basic line height. */ | ||
--pgn-typography-font-weight-display-4: var(--pgn-typography-font-weight-bold); /* Font weight of display level 4. */ | ||
--pgn-typography-font-weight-display-3: var(--pgn-typography-font-weight-bold); /* Font weight of display level 3. */ | ||
--pgn-typography-font-weight-display-2: var(--pgn-typography-font-weight-bold); /* Font weight of display level 2. */ | ||
--pgn-typography-font-weight-display-1: var(--pgn-typography-font-weight-bold); /* Font weight of display level 1. */ | ||
--pgn-typography-font-weight-table-th: bold; /* Table th font weight. */ | ||
--pgn-typography-font-weight-table-th: 700; /* Table th font weight. */ |
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.
[inform] sd-transforms
changes bold
to the corresponding, explicit font weight.
97c585b
to
2667256
Compare
@@ -56,9 +57,14 @@ async function buildTokensCommand(commandArgs) { | |||
source: tokensSource | |||
? [`${tokensSource}/core/**/*.json`, `${tokensSource}/core/**/*.toml`] | |||
: [], | |||
preprocessors: ['tokens-studio'], | |||
expand: { | |||
typesMap: (await getTokensStudioTransforms()).expandTypesMap, |
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.
[inform] See sd-transforms
README for more information on expandTypesMap
for expand
.
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.
Relates to #3222
2667256
to
9e458f0
Compare
@@ -15,7 +15,7 @@ | |||
}, | |||
"columns": { | |||
"margin": { "source": "$card-columns-margin", "$value": "{spacing.card.spacer.y}" }, | |||
"count": { "source": "$card-columns-count", "$value": "3" }, | |||
"count": { "source": "$card-columns-count", "$value": "3", "$type": "number" }, |
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.
[inform] Bug fix around $type
for a number value, as this token does not define a dimension type currently defined above.
4edbef8
to
644bb59
Compare
themes.forEach((themeVariant) => { | ||
StyleDictionary.registerFilter({ | ||
name: `isThemeVariant.${themeVariant}`, | ||
filter: token => token.filePath.includes(themeVariant), | ||
}); | ||
}); | ||
|
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.
[inform] Duplicate. Already defined in paragonFilters
above.
27191be
to
e07b067
Compare
e07b067
to
338109f
Compare
338109f
to
f4c2332
Compare
@@ -56,9 +57,14 @@ async function buildTokensCommand(commandArgs) { | |||
source: tokensSource | |||
? [`${tokensSource}/core/**/*.json`, `${tokensSource}/core/**/*.toml`] | |||
: [], | |||
preprocessors: ['pgn-annotate-token-extensions-with-references', 'tokens-studio'], |
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.
[inform] See docs on preprocessors. pgn-annotate-tokens-with-references
is custom.
See sd-transforms
README for more information on the tokens-studio
preprocessor.
@@ -3,7 +3,7 @@ | |||
"$type": "dimension", | |||
"breakpoint": { | |||
"xs": { | |||
"$value": "0", | |||
"$value": "0px", |
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.
[inform] Ensures consistency in unit with other breakpoints.
f4c2332
to
75f88ef
Compare
…tput; resolves refs warnings
75f88ef
to
fbb084b
Compare
'org.openedx.paragon': { | ||
...referencedToken.$extensions?.['org.openedx.paragon'], | ||
[propertyName]: true, | ||
}, |
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.
See https://tr.designtokens.org/format/#extensions for more details about $extensions
. It's being used in this implementation to help include references tokens in build output, even if the original filtering doesn't apply.
E.g.,
{
// token metadata like $value, etc.
$extensions: {
'org.openedx.paragon': {
isReferencedBySourceToken: bool, // useful to include referenced non-source tokens alongside source-only tokens
isReferencedByThemeVariant: bool, // useful to include referenced non-theme variant tokens alongside theme variant tokens
},
},
}
This is the "magic" behind suppressing the (intentional) previous output reference warnings during build.
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.
Overall this looks great! I'd be a bit more confident if there were some tests for the new functions in utils.js
, but I'm on the fence about that being a blocking requirement. There might be other things that have already landed in the design tokens branch that might be missing tests, and in that case I think making an issue to add tests and just adding this new functions to the list of tests to add could work.
tl;dr
- Looks good!
- Tests?
- Should probably comb through all of the new design tokens functionality and see if there are untested places we should add tests (out of scope for this PR)
@brian-smith-tcril Thanks for the review! Agreed the lack of tests isn't ideal to ensure confidence in changes. However, as far as I can tell, there are no current tests for any of the logic defined in the |
I made an issue for adding tests #3229, we can add to it as we find places where tests are missing. |
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.
daffd74
into
Peter_Kulko/style-dictionary-v4
Description
style-dictionary
withsd-transforms
(GitHub) from Tokens Studio. Enables support for copying exported tokens from the Tokens Studio plugin in Figma and adding them to the Paragon tokens schema.paragon-css
to abstract away our custom transforms,sd-transforms
, and the default CSS transforms. The previously excludedsize/rem
filter for the default CSS transforms is added back in.light
theme variant tokens that referencecore
tokens. Now, these handful referencedcore
tokens are included in thelight
theme CSS variables, although duplicative of their definitions incore.css
; this is an acceptable tradeoff to remove noisy, disruptive warnings as it only expands the contents of output files by the number of referenced tokens.@edx/elm-theme
), this use case applies when eithercore
and/orlight
tokens reference base, default Paragon tokens without redefining the base token within the brand package. Now, the referenced default Paragon token(s) are included in the brand package output.build-tokens:watch
NPM script to automatically execute thebuild-tokens
CLI on file changes, enabling more rapid development when changing tokens and verifying output without having to manually re-runbuild-tokens
yourself.Deploy Preview
Resolves output references warning in build output
Related to (3) above. Paragon tokens output on left; brand package tokens output on right (see related
@edx/elm-theme
PR).Before
After
Merge Checklist
example
app?Post-merge Checklist