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

Support for references without .value #723

Closed
wants to merge 5 commits into from

Conversation

markacianfrani
Copy link
Contributor

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

@chazzmoney
Copy link
Collaborator

chazzmoney commented Oct 28, 2021

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!

@markacianfrani
Copy link
Contributor Author

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')) {
Copy link
Contributor Author

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.

@markacianfrani
Copy link
Contributor Author

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 color.brand.secondary, it causes all sorts of side effects. Mixing and matching using .value and not using .value is going to be difficult I think. Although, I really don't see a legit use case where you would want to be able to use both at the same time.

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.

@JackHowa
Copy link

this looks like it'd be really helpful!

@lukasoppermann
Copy link
Contributor

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

@dbanksdesign ?

@dbanksdesign
Copy link
Member

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.

@dbanksdesign
Copy link
Member

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!

@dbanksdesign
Copy link
Member

Closing this in favor of #746 but we will give you credit in the release notes @markacianfrani!

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

6 participants