design-tokens
design-tokens copied to clipboard
Add setting option to exclude mode from token value
- new setting to allow include the mode name in the variable path, variable values (when alias), or neither of them. This setting inly applies when more than one mode is present in a collection.
- add documentation in README.md
- extend Checkbox component to embed Info component inline to the checkbox label
- update jest moduleMapper to resolve "@src"
Fix #308
@lukasoppermann hi! I am wondering if I am supposed to take any further step to have this PR approved or declined. I am not familiar with the process, thus I am asking 😀
Hey @0m4r,
this is good, I just have to review it.
I will probably remove the contributors section.
I am happy for it to be added, but then we need to do a new PR and add all people who contributed.
Hey @0m4r I added a PR here: https://github.com/0m4r/design-tokens/pull/1
But it is still, not working. The name is not shown in the JSON structure.
(I am not close to a computer to test at the moment)
But, could you help me to replicate your settings with a screenshot?
The "new" behavior should be setup by ticking off the "old" checkbox, and tick on the "new" one.
...now that I think about it may not be the best user experience, tho 😓
Hey, I updated the naming a bit but this does not matter.
However, no matter if the new setting is on or off, it never adds the mode to the name.
Adding to the name for me means it should be in the json structure
{
base: { // <- (collection)
light: { // <- (mode)
green: {...}
This is also how the name is referenced in the value (if the new setting is on). But this does not happen.
I expect (but could not test it) that we are missing something in the export or somewhere. I also did not log the name to see if it is actually added.
Just FYI I am on vacation for two weeks very soon, so I may not answer for two weeks.
I have run a few tests and reported the findings here: https://github.com/0m4r/design-tokens/pull/1#issuecomment-2242563641 what do you think?
Hey @0m4r are you still on this? I agree that the UX seems a bit confusing. Any idea how to fix it?
Hi Lukas, I am still interested to fix this, but in the last weeks I have not really got a lot of time to spend on this.
Let me come back to you with some ideas... (Especially about the issue of referencing variable in the same mode - that seems to be the weirdest issue so far)
hi @lukasoppermann , I came up with a solution that seems to work for the most common use cases. There are 2 options now:
- use the mode name in the token name
- use the mode name in the token value
by combining them, name and value can both be generated including the mode name, only one of them or none.
The problem with values that reference variables in the same mode remains unresolved. I am not able to figure out now if and how this information is available to be detected.
(I have not updated screenshots and docs yet - I wanted to have a review of the current idea before completing all the work )
Pull Request Test Coverage Report for Build 11630838963
Details
- 8 of 31 (25.81%) changed or added relevant lines in 3 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+1.6%) to 70.757%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| src/ui/modules/downloadJson.ts | 0 | 5 | 0.0% |
| src/utilities/getVariables.ts | 6 | 24 | 25.0% |
| <!-- | Total: | 8 | 31 |
| Totals | |
|---|---|
| Change from base Build 10947804742: | 1.6% |
| Covered Lines: | 584 |
| Relevant Lines: | 809 |
💛 - Coveralls
@lukasoppermann I have now completed the work on this PR and updated the README.md. It is now also resolving the alias within the same collection/mode.
The code is not great, I know... still, what do you think?
Hey @0m4r,
sorry for the late reply. I added a few comments. It looks great!
However, it seems that this PR introduces a memory leak.
You can see it if you run it locally. I ran it in a project with a lot of tokens and modes. It runs fine with only one mode.
I did not test if just adding a mode breaks it already, but it may.
Don't know if you have an idea. I don't know why it breaks.
Hey @0m4r,
sorry for the late reply. I added a few comments. It looks great!
However, it seems that this PR introduces a memory leak.
You can see it if you run it locally. I ran it in a project with a lot of tokens and modes. It runs fine with only one mode.
I did not test if just adding a mode breaks it already, but it may.
Don't know if you have an idea. I don't know why it breaks.
Uhm, I did try with a few different Figmna projects, but none of them is really "huge". I will be looking into it - I tried right now on my biggest test project and I was not able to replicate... it may take a moment to fix, at least until I do not manage to replicate. The only think I can think of is the new function that iterates variables to figure out aliases in the same collection or mode.
Could be this. I have aliases from the same collection and from collections in other libraries.
Happy to test again. If I can just disable the function to test if it is this one, let me know how
yeah, that would help... (and, to be honest, I have not even considered the use case of variables in other libraries...)
@lukasoppermann the easiest way is probably to edit detectVariableReferencesInCollection in src/utilities/getVariables.ts to immediately return variable (of course, the same collection/mode variables will not be resolved).
(worth asking, I guess, I am trying to generate a huge design token file, what's the best way to import it back to Figma as Figma variables? I am trying some plugins but none of them does work...)
@lukasoppermann I found a fix that should work around the memory leak issue for now. I tested it on the "Premier Web (Community)" figma variables and it does not break - but it takes quite some time tho)
let me know what you do think about this change
Hey @0m4r I am still running into the issue. I tried to make some time to debug, but I am too blocked atm. Do you have an idea what creates this memory leak?
Hey @0m4r I am still running into the issue. I tried to make some time to debug, but I am too blocked atm. Do you have an idea what creates this memory leak?
Hi @lukasoppermann , yeah, the problem is that when it tries to "resolve" the same mode or same collection variable reference it iterates the current variable against any other. As far as I can tell, as of now, this is the only way to detect this situation (happy to be told wrong on this!). The problem is that when the numbers are huge, this generates a memory leak - and when it succeeds it takes anyway a huge amount of time.
I am going to check this one again, but I think I would like to add an additional "beta flag" to enable resolving the same mode/collection variable references.
This way, at least the initial problem of the mode names in the token name or value can probably be pushed forward. The same mode/collection reference resolution can be avoided by structuring the Figma variables differently, so it is not really a blocker but more like a nice to have.
Hey @0m4r,
so it is better. Only if I set Add mode to design token name (if 2 or more modes) to true there is an issue.
It does nothing and just closes. I think if you can bring up a Figma error notification and abort, we could at least merge it. The exact error should be output to the console. And for the user we need to just out put something like:
Error exporting tokens
I currently don't understand why this happens, so I would not know a better error code. But I think we can fix this error in a second PR.
We just need to deal with the error for now.
Hey @0m4r,
so it is better. Only if I set
Add mode to design token name (if 2 or more modes)to true there is an issue.It does nothing and just closes. I think if you can bring up a Figma error notification and abort, we could at least merge it. The exact error should be output to the console. And for the user we need to just out put something like:
Error exporting tokensI currently don't understand why this happens, so I would not know a better error code. But I think we can fix this error in a second PR.
We just need to deal with the error for now.
@lukasoppermann I am not able to replicate the problem. I tried all the possible switches combinations for the mode name but I have got no errors. Can you share some more information?
@0m4r there is no error, it just does nothing. I am assuming you abort due to some error?
As you can see in the video, it works fine with the left option disabled. I waited for a couple minutes, but nothing happens once the option is enabled and I export.
https://github.com/user-attachments/assets/aa8656ee-b7e6-4dbd-85c9-e7bbe67b405f
Thanks for the video, I have been trying before to replicate the error you have shown in the recording but it did work for me. I am gonna give it another try, hopefully, I can replicate it and either fix it or prompt the error.
@0m4r I don't know if it is related to the tokens referencing tokens from another file?
@0m4r I don't know if it is related to the tokens referencing tokens from another file?
I am looking into it. I have noticed that if I use the plugin in watch mode (npm run start) it all seems working. but when I use the built version of it (npm run build), then the problem you report is showing.
@lukasoppermann I think I have nailed it down to where the issue occurs.
It is in downloadJson.ts. I am not 100% confident, but I believe the problem lies in between the programmatic click to the anchor element to trigger the download (that takes some time with a large amount of information) and the postMessage that triggers the command: commands.closePlugin,. So that the plugin closes before the work is done.
I have tried changing a bit the code to optimize the way the data to download is generated (moved away from encodeURIComponent in favor of Blob and createObjectURL). This seems to perform better (or faster!).
If this solution does not work. Some more work needs to be done to eventually add a second "Close" button next to "Export", so to allow the user to more intuitively close the plugin.
The problem remains with the notification it is not possible to trigger it once the file download is completed, at least with the current paradigm of downloading the data itself.
https://github.com/lukasoppermann/design-tokens/pull/309/files#diff-5753530eeb35df6a7b9b334128eb3b14823849f79633dfb54a12af5e73a72218R20
(code pushed)
I am sorry for it taking so long. It is nearly perfect. We just need to change one thing.
ATM when I open the plugin the json is generated, even if I open the settings. This means once the setting is turned on I am stuck.
This can easily be solved by updating the index.ts file.
Replace this
// write tokens to json file
figma.ui.postMessage({
command: figma.command as PluginCommands,
payload: {
settings: {
...userSettings,
...{ accessToken: await getAccessToken(getFileId(figma)) }
},
data: stringifyJson(exportRawTokenArray(figma, userSettings)),
versionDifference: versionDifference,
metadata: {
filename: figma.root.name
}
}
} || {} as PluginMessage)
With this:
if([commands.export, commands.urlExport].includes(figma.command as PluginCommands)) {
// write tokens to json file
figma.ui.postMessage({
command: figma.command as PluginCommands,
payload: {
settings: {
...userSettings,
...{ accessToken: await getAccessToken(getFileId(figma)) }
},
data: stringifyJson(exportRawTokenArray(figma, userSettings)),
versionDifference: versionDifference,
metadata: {
filename: figma.root.name
}
}
} || {} as PluginMessage)
} else {
// write tokens to json file
figma.ui.postMessage({
command: figma.command as PluginCommands,
payload: {
settings: {
...userSettings,
...{ accessToken: await getAccessToken(getFileId(figma)) }
},
versionDifference: versionDifference,
metadata: {
filename: figma.root.name
}
}
} || {} as PluginMessage)
}
I did notice this problem too but I thought it to be "a problem for another time" to avoid adding too much in here. ...but as you already have a solution I will add it. Thanks for the code snippets!
I did notice this problem too but I thought it to be "a problem for another time" to avoid adding too much in here. ...but as you already have a solution I will add it. Thanks for the code snippets!
@lukasoppermann this is ready for review too.
Perfect, thank you for all the work. Sorry that it was such a long process
