fix: don't overwrite linked style with raw value

[jakobe]

Fixes #758

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
figma-tokens	✅ Ready (Inspect)	Visit Preview	Sep 5, 2022 at 6:51PM (UTC)
ft-storybook	✅ Ready (Inspect)	Visit Preview	Sep 5, 2022 at 6:51PM (UTC)

[jakobe]

Hi @six7 ,

I've cleaned up the code and moved all new functions to separate files as suggested - as well as removed some code duplication.

I've tried to follow the current structure, i.e. added to/updated figmaTransforms, figmaUtils and utils folders as appropriate. Let me know if you think anything needs to move somewhere else.

I also took the liberty to refactor the isPaintEqual method as it actually doesn't work at the moment when comparing Figma's rgba values to web rgba.

Finally I've fixed the failing tests (mocks missing get|setSharedPluginData methods).

The refactoring to own files are done in this commit https://github.com/six7/figma-tokens/pull/765/commits/62d25347468dff9c987fecfacd8ae5376c918f2b and this https://github.com/six7/figma-tokens/pull/765/commits/a4ed028ebb23267a9467a1cafd63db5bfc179cac

I consider this PR to be pretty done now, but I'll leave it as a draft PR until you ping me about the theme style ids PR.

Cheers, Jakob 😊

[six7]

Hey @jakobe - we finally merged the theme / style id connections changes into next. Do you mind merging next into your branch and giving it a go if it still works? Thank you 🎉

[jakobe]

Hey @jakobe - we finally merged the theme / style id connections changes into next. Do you mind merging next into your branch and giving it a go if it still works? Thank you 🎉

Hey @six7 great to hear 🎉 Sorry for the late reply, just came back today after a long and needed vacation 😎 I'll have a look today or tomorrow and get back to you 🤞🏻 Cheers - J

[jakobe]

Hey @jakobe - we finally merged the theme / style id connections changes into next. Do you mind merging next into your branch and giving it a go if it still works? Thank you 🎉

Hey @six7, I merged the latest next branch into my fork and solved all merge conflicts 👍🏻 It seems to still work in the testing I've done myself - I've also asked my designer to do some additional testing on some of his work as well.

Would you or @LiamMartens have time to have a look at the PR and let me know if you see any improvements I should make? Thx, Jakes 😄

[six7]

@jakobe could you fix the merge errors that seem to have occurred? We renamed a type which seems to cause that.

@LiamMartens could you look at this PR from a high level? It's solving the case where a user does not have use the NonLocal styles feature but wants to retain the style connection on a layer, right now we always change to hex even if there was no change in the token value, however it would be ideal if we'd just keep that style set on the layer if possible, which this PR does.

[jakobe]

@jakobe could you fix the merge errors that seem to have occurred? We renamed a type which seems to cause that.

@LiamMartens could you look at this PR from a high level? It's solving the case where a user does not have use the NonLocal styles feature but wants to retain the style connection on a layer, right now we always change to hex even if there was no change in the token value, however it would be ideal if we'd just keep that style set on the layer if possible, which this PR does.

Oh thx, didn't catch that one. Seems you fixed a typo on next branch after I merged it - it's fixed now 👍🏻

@LiamMartens Also note, that this solves going the other way as well, i.e. if the plugin has overriden an applied non-local style with raw token values (e.g. selecting a dark theme in the plugin) then when going back to tokens that matches a non-local style (e.g. default/light theme) this will re-connect/apply the non-local style instead of the raw values.

[jakobe]

@jakobe looks good overall - added a few minor questions. Aside from that it would be great to add test coverage for the new utilities like the style matchers etc...

Good point, I'll have a look at adding tests 👍🏻

[jakobe]

@jakobe looks good overall - added a few minor questions. Aside from that it would be great to add test coverage for the new utilities like the style matchers etc...

Hey @LiamMartens , added a bunch of tests to all the new style matchers util classes: https://github.com/six7/figma-tokens/pull/765#commits-pushed-246c7bd

Also added the linear gradient color matcher, removed the description from text style matcher and added some notes to your question regarding style id backup.

Will you give it a re-review? Thx 🙏🏻 😊

[jakobe]

@LiamMartens & @six7 any news on this one? 🙏🏻 😊

[jakobe]

@six7 @LiamMartens I did a little tweak yesterday to the code that makes the backup of the styleid before it overwrites the non-local style with raw values.

I'll try to explain:

It now checks if the path of the selected token matches the name of the non-local style, before it backs up the style id and overwrites it with raw values (e.g. when selecting a dark mode tab): {color.action-cta.default} => color/action-cta/default and {color.action-cta.default} => action-cta/default (when ignoring first part of token name for styles)

Otherwise it would keep "stale" styleids when assigning a different token, because the new token wouldn't match the linked non-local style.

TL;DR: We should only backup the linked non-local style when the value doesn't match but the token path matches the style name, because that signals it's the same token as the linked style but with a different value (e.g. dark mode).

Hope it makes sense 😊 🤞🏻

[LiamMartens]

@six7 I think this looks good from a code perspective - not sure what you want to do with the coverage diff situation. Did you check this out functionally?

[six7]

@six7 I think this looks good from a code perspective - not sure what you want to do with the coverage diff situation. Did you check this out functionally?

I'll give this a functional go later this week 👍

Regarding test coverage checks: Unfortunately our integration doesn't seem to be working for external collaborators :/ I'll check manually if coverage for added files increased or not

[six7]

Merging this in, checked functionality 🙏

We'll be releasing a beta for version 120 on Saturday which we're aiming to test for a week before releasing.

[jakobe]

Merging this in, checked functionality 🙏

We'll be releasing a beta for version 120 on Saturday which we're aiming to test for a week before releasing.

Great to hear, thanks a lot @six7 🙏🏻

Sounds good with v120 and the timeline 👍🏻 😊