-
Notifications
You must be signed in to change notification settings - Fork 566
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(references): ability to reference other tokens without 'value' #746
Conversation
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.
A few comments to explain some changes.
@@ -18,7 +18,7 @@ const {buildPath} = require('./_constants'); | |||
describe('integration', () => { | |||
describe('android', () => { | |||
StyleDictionary.extend({ | |||
source: [`__integration__/tokens/**/*.json`], | |||
source: [`__integration__/tokens/**/*.json?(c)`], |
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 allows our token files to be .json
or .jsonc
. I made this change so I could add a comment in a token file about testing the use of leaving off .value
.
"danger": { "value": "{color.core.red.1000.value}" }, | ||
"warning": { "value": "{color.core.orange.1000.value}" }, | ||
"success": { "value": "{color.core.green.1000.value}" } | ||
// make sure references without .value work too | ||
"success": { "value": "{color.core.green.1000}" } |
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.
👀
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.
👀
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.
Hmmmmmm..... its ok, I guess.
It is kind of exciting to get this into the project. It is also a bit disconcerting to me. Things that stand out in my mind as worrisome:
Anyway, I'm excited about this. And worried. But mostly excited. |
@chazzmoney added some more tests for. Now we are testing to make sure referencing a non-token still works in a number of different ways (by itself, interpolated, part of an object) as well as other non-token cases. Also the tests I wrote are a bit easier to read now instead of just comparing 2 objects, just testing for what we want to know. |
Hello, sorry, for nudging, but is there an ETA for this PR? I would love to wait with releasing an update form my figma plugin so that I don't have to include an option to add a But I don't want to block the release for too long. Would be great if you could give me an idea when this will be shipped. Thank you. |
@lukasoppermann Ta-Da! |
@chazzmoney awesome. Could you also create a release so I can start using it. 🙏 |
|
I'm all for keeping up with the W3C spec, but this is definitely a breaking change: My project has tokens that reference other tokens in a way that's handled by a custom formatter. It's convenient to be able to omit For the sake of anyone else running into this issue when upgrading to // In style-dictionary < 3.1.0
import type {Dictionary} from 'style-dictionary/types/Dictionary';
function myFormatter({dictionary}: {dictionary: Dictionary}) => {
// get token from dictionary...
useTokenReference(token.referenceToOtherToken);
// ...
} // In style-dictionary >= 3.1.0
import type {Dictionary} from 'style-dictionary/types/Dictionary';
function myFormatter({dictionary}: {dictionary: Dictionary}) => {
// get token from dictionary...
const otherToken = dictionary.getReferences(token.referenceToOtherToken)[0];
useTokenReference(otherToken);
// ...
} It'd be helpful if there were a way to clearly differentiate "references to tokens" from "references to token values" in the actual token documents. Maybe there could be a syntax like |
@TrevorBurnham Thank you for saying this! I was worried about someone having a use case that we interfered with, and you’ve stated it clearly. @dbanksdesign and I will make this one a priority. |
@chazzmoney Thanks, I appreciate the response! FWIW here's the specific way that the referenced token is used in my project: We construct the names of CSS variables using the full token path, just like the {
"color": {
"red": {"value": "#ff0000"}
}
} yields CSS output like // Output from css/variables formatter
--color-red: #ff0000; What's special about our case is that we have a format that includes the CSS variable names of referenced tokens. So: {
"errorColor": {"value": "{color.red}"}
} builds to // Output from custom formatter
{
"errorColor": "--color-red"
} |
I figured out an (arguably) more elegant solution for my use case: I can use an attribute transform to add an attribute to the token based on its path (just like the So with the above example, that would mean:
{
"errorColor": {"value": "{color.red.attributes.cssVarName}"}
} That seems to work perfectly well, and I like that it makes what the reference is doing more explicit. The only thing that makes me nervous is that AFAICT the ability to reference |
Issue #, if available: #721
Description of changes: Per the draft W3C design tokens spec, references do not need
.value
in them. This change should be a backwards-compatible change (you can still use.value
) to support the spec. You can now reference other tokens without.value
!https://design-tokens.github.io/community-group/format/#aliases-references
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.