Skip to content
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

Merged
merged 2 commits into from
Dec 15, 2021

Conversation

dbanksdesign
Copy link
Member

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.

Copy link
Member Author

@dbanksdesign dbanksdesign left a 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)`],
Copy link
Member Author

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}" }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Collaborator

@chazzmoney chazzmoney left a 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.

@chazzmoney
Copy link
Collaborator

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:

  • If someone has an SD systems containing tokens which are NOT properties (specifically, with no value specified), but maybe used for other metadata purposes
  • If someone somehow set up child properties underneath a parent property that had a value...
  • I really really REALLY think we need to add property typing. Like with this change EVERYTHING is a property. But if we knew that something is a color property then we know a bunch of things we can do with it. This removes the only tiny bit of typing we have in SD right now (separating hierarchy from properties). With typing, value isn't needed but we can distinguish what things are. There is a lot to say, but I'll stop here.
  • I don't know, it just gives me some angst...

Anyway, I'm excited about this. And worried. But mostly excited.

@dbanksdesign
Copy link
Member Author

@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.

@lukasoppermann
Copy link
Contributor

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 .value.

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.

@chazzmoney chazzmoney merged commit c6f482e into main Dec 15, 2021
@chazzmoney
Copy link
Collaborator

@lukasoppermann Ta-Da!

@lukasoppermann
Copy link
Contributor

@chazzmoney awesome. Could you also create a release so I can start using it. 🙏

@chazzmoney
Copy link
Collaborator

@chazzmoney awesome. Could you also create a release so I can start using it. 🙏

Done

Robbert added a commit to nl-design-system/themes that referenced this pull request Jan 4, 2022
Robbert added a commit to nl-design-system/denhaag that referenced this pull request Jan 4, 2022
Yolijn pushed a commit to nl-design-system/themes that referenced this pull request Jan 4, 2022
Robbert added a commit to nl-design-system/denhaag that referenced this pull request Jan 6, 2022
robbieatacato pushed a commit to nl-design-system/denhaag that referenced this pull request Jan 11, 2022
@TrevorBurnham
Copy link
Contributor

This change should be a backwards-compatible change

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 .value most of the time, but sometimes you do need the full token, e.g. because the formatter cares about which group the referenced token belongs to.

For the sake of anyone else running into this issue when upgrading to style-dictionary@3.1.0, here's the fix I'm using, where a token is of the form {"referenceToOtherToken": "{other.token.path}"}:

// 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 {"referenceToOtherToken": "{!other.token.path}"} to mean "Yes, I really want to referenceToOtherToken to resolve to the whole token." Of course, I can understand wanting to defer to the W3C spec on this point. I'll see if I can start a discussion there.

@chazzmoney
Copy link
Collaborator

@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.

@TrevorBurnham
Copy link
Contributor

@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 css/variables format does. So e.g.

{
  "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"
}

@TrevorBurnham
Copy link
Contributor

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 attribute/cti transform), then reference that attribute instead.

So with the above example, that would mean:

  1. I add a transform that adds the attribute token.cssVarName = '--color-red to the color.red token, and
  2. I change the reference to
{
  "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 attributes in token files is undocumented. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants