-
Notifications
You must be signed in to change notification settings - Fork 558
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: add preExpand meta info to expanded tokens #1269
base: main
Are you sure you want to change the base?
Conversation
513fa06
to
7832e96
Compare
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 change seems small, I think it warrants a larger discussion. This is the first time we are using $extensions
part of the design token. Once we add this in, it will be difficult to remove.
I would like to see some docs added in this PR as well. It would be helpful to see an example in docs that lays out the user problem this is solving.
$extensions: { | ||
'com.styledictionary': { | ||
preExpand: { | ||
type: compositionType, |
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.
Should this be compositionType
to be more explicit?
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.
compositeType
I guess? Although both is probably fine
// store the original composite token type as well as which prop it was | ||
// so that for example a lineHeight property of a typography token, when expanded | ||
// can still be matched as a lineHeight token, even though its type is now "number" after the expansion | ||
$extensions: { |
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.
This is the first time Style Dictionary is using $extensions
. Do we expect to put more in here later?
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.
I'm thinking that perhaps it makes sense to move other properties that Style Dictionary adds to the token metadata under this namespace, as not to clash with metadata that the token author themselves may add. This could be planned for a future v5 I reckon
I'll add some docs on this, I agree it makes sense to be clear about use cases 👍🏻 |
7832e96
to
09dd1f5
Compare
Hey @dbanksdesign I added some docs, and while describing the use case of letterSpacing I started to realize that maybe we don't need to go through with this PR. I'm arguing that e.g. for So essentially, this token has 2 types in a sense. But that's kinda stupid imo because a standalone token of type "letterSpacing" could never have 2 types, so expanded tokens would be unique in that capability; this doesn't make much sense. So if a DTCG extension spec decides there are types that don't exist (yet) in DTCG, they just have to add them to the expandTypesMap: {
expand: {
typesMap: {
typography: {
letterSpacing: 'letterSpacing'
}
}
}
} And then the expanded StyleDictionary.registerTransform({
...StyleDictionary.hooks.transforms['size/rem'],
filter: (token, options) => {
const type = options.usesDtcg ? token.$type : token.type;
// original only applies to dimension and fontSize
return ['dimension', 'fontSize', 'letterSpacing'].includes(type);
}
}) This overrides the built-in Conclusion: I'd like to close this PR since I don't see an urgent need for it anymore. I'll raise an issue to document the above thing better. |
#1334 @dbanksdesign I found a use case for this PR to be reopened |
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Issue #, if available:
A user has told me that it's kinda difficult for him to apply custom transforms to typography token properties such as letterSpacing or lineHeight, because after expanding them they are typed as
dimension
/number
respectively, and therefore it's hard to set a filter function that matches only letterSpacing/lineHeight tokens when the types have been aligned / reduced to DTCG types.Description of changes:
Add metadata under
$extensions.['com.styledictionary']
(where metadata should go from now on, following DTCG guidelines) so you can set a transform filter that targets expanded typography -> lineHeight tokensBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.