-
Notifications
You must be signed in to change notification settings - Fork 580
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
Support for references without .value #723
Conversation
In the case that value comes in as an object, we can check if it has a value property and use that re #721
Hi @markacianfrani , thank you for your contribution! I like where we are headed here, but there is a touch more to do. Can you add some tests to ensure that the reference system is all processed as expected - both when value is (and is not) included? And tests to ensure that tokens are correctly resolved whether outputReferences is set or not. Thank you! |
Well, the good news is the test suites are really good and the bad news is....the test suites are really good. I see now that this might be way more complicated than I originally thought. It's the formatters that are giving me trouble now (compose/object for starters). Back to the drawing board! |
* If .value is omitted from the input | ||
* ie value: "{color.core.purple}" instead of "{color.core.purple.value}" | ||
*/ | ||
if (properties[name].value.hasOwnProperty('value')) { |
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 don't love putting this here, since this is flattenProperties function is supposed to be as generic as possible.
Had to go way farther up the chain for this. Doing it at the format level caused all sorts of headaches. I did find another edge case--in the current test suite, if you were to change, color.font.interactive.active to But is this even something we want to solve at the Style Dictionary level? I'm totally okay with closing this and solving it at the Figma Tokens Plugin level instead. |
this looks like it'd be really helpful! |
Hey, can this be merged? I am working on references for my figma plugin which exports mainly for style dictionary. However I would love to keep it in line with the w3c spec. If thus could be merged I wouldn't need to include some hacky solution that adds |
Hey everyone! Sorry I haven't been responsive on this PR, my team just had a big launch at re:invent last week that took all of my attention. I am taking a closer look at this PR today and I would like to get this merged in this week and hopefully do a release. |
I took a look at this and figured it would be easier to make the change because it requires change code in a more core place. @markacianfrani I'll list you as the co-author though to give you credit. Take a look at #746 and thanks for your patience! |
Closing this in favor of #746 but we will give you credit in the release notes @markacianfrani! |
#721
The issue above does a good job explaining the problem. If you omit the
.value
in your token format (as is the case when using the Figma Tokens plugin and also the W3 spec), SD fails the reference stage and results in [object object]. Looking at the object returned if you have this omission, there should be a .value property that you can just reference.However, this seems way too simple so I'm wondering what edge cases or scenarios I might be overlooking here?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.