fix: ignore variable mode value if it is a variable alias #20
Conversation
|
Appreciate the example project you put together @james04321! 🎖️ In our variables setup we have two libraries (Primitives, Themes). Variables in the themes library reference variables in the primitives library. When I tested this in the context of our project the script Please correct me if the code suggested for ignoring these variables does not make sense or if we should fix this some other way. Cheers 🍻 |
|
Thanks for bringing this up @torleifhalseth! I think we forgot to test for aliases to remote variables. I'll look at this later this week when I get the chance. Does this pass the existing tests? |
|
Yes, existing tests are passing ✅ |
…exclude these from the post variables payload
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ble mode values that are missing
|
When should we add a variable value under a specific mode value to the POST payload? Scenario 1: Scenario 2: How would we know if a token in file should create a new Figma Variable or if it is an external reference? For now, i created the |
My immediate intuition (that I haven't thought through fully yet) is that we should include remote variable aliases in However, we are limited by the API in that we can't bring in remote variables that don't already live in the current file. When I have time later (probably next week), I want to think through how the export/import scripts work with remote variables to make sure we're handling them consistently. In the meantime, feel free to make whatever changes you need in your own fork if you're blocked by this. We might not end up using the changes in this PR depending on where we land, so don't feel like you have to polish everything right now. |
|
Sounds like a good idea @james04321 . I will leave this for now and wait for your feedback/solution for working with remote variables. |
@torleifhalseth Made a separate PR to fix this issue, which allows us to update aliases to remote variables. Let me know if this works for you: #23 |
…yncing tokens to Figma (#23) For the `sync_tokens_to_figma.ts` script, if a tokens file has aliases to variables in remote collections, e.g. `"$value": "{name_of_variable}"`, update those alias references when needed. This issue was brought up in #20. The previous behavior was that if a tokens file has aliases to variables in a remote collection, those aliases would cause a 400 response from the `POST /v1/files/:file_key/variables` endpoint, as the script was sending up invalid variable ids inside the alias objects. Now, the script will match up token values with remote variable names and update aliases when needed. Note: we are limited by the variables API in that we cannot alias variables that aren't already consumed by current file. Demo: Here is a demo of the `sync_tokens_to_figma.ts` script updating a Figma file that has a Semantic variables collection with aliases to variables published from another file. https://github.com/figma/variables-github-action-example/assets/250513/592fddbc-38fe-485c-89ca-688b1777b64f ## Test Plan - In one file, create and publish a variable collection - In another file, create a variable collection with variables that alias to variables from the file above - Run `npm run sync-figma-to-tokens` to export the variables from the second file into a tokens file - Run `npm run sync-tokens-to-figma` on the exported tokens file to sync the tokens back to Figma. The result is that we should noop instead of getting a 400 error from the REST API.
|
Great work @james04321 🎖️ The solution works great for us. Thank you for taking the time to follow up and fixing this. For now, we are duplicating this code in our project, but it would be great if this was packaged at some point. Is that something you guys would consider @james04321? The same way you did with the |
😍 Glad you are finding the spec useful!
We don't have plans to do this primarily because there are a bunch of opinionated design decisions in the code such as the format of the tokens json, intentionally conservative behavior (not removing variables in the Figma file that have been removed in the tokens file), etc... and we want organizations using this code to be intentional about how they're setting up their Figma-GitHub sync to avoid messing up their Figma files. |
|
Ok. I understand. I guess the opinionated design decisions could be configurable, but then the code complexity increases 🤷♂️ This example is a great starting point for developers, and I agree that we should be intentional about how we are setting up the sync. I also think the majority of designers and developers appreciate opinionated decisions when they are described and well thought over 🚀 |
If the variable mode value is referencing a remote variable we should ignore it when generating the post variables payload.