Add setting option to exclude mode from token value #309

0m4r · 2024-07-05T14:00:24Z

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"

@src

* new setting to allow to only preserve mode in token name but exclude it from token value * add documentation in README.md * extend Checkbox component to ember Info component inline to the checkbox label * update jest moduleMapper to resolve "@src"

0m4r · 2024-07-18T08:44:24Z

@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 😀

lukasoppermann · 2024-07-18T16:15:37Z

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.

lukasoppermann · 2024-07-19T07:55:10Z

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.

0m4r · 2024-07-19T08:32:42Z

(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 😓

lukasoppermann · 2024-07-19T08:39:18Z

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.

0m4r · 2024-07-22T10:09:40Z

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?

Some updates but not working

lukasoppermann · 2024-09-19T19:45:49Z

Hey @0m4r are you still on this? I agree that the UX seems a bit confusing. Any idea how to fix it?

0m4r · 2024-09-19T19:52:36Z

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)

Add setting configuration to add the mode name to the token name or value. Both token name and value can have the mode added, only one of them or none.

0m4r · 2024-09-26T20:16:10Z

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 )

coveralls · 2024-09-30T09:40:37Z

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%

Totals
Change from base Build 10947804742:	1.6%
Covered Lines:	584
Relevant Lines:	809

💛 - Coveralls

0m4r · 2024-09-30T15:26:03Z

@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?

lukasoppermann · 2024-10-07T22:09:55Z

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.

0m4r · 2024-10-08T06:01:46Z

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.

lukasoppermann · 2024-10-08T06:31:04Z

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

0m4r · 2024-10-08T06:39:30Z

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 · 2024-10-08T07:02:39Z

src/utilities/getVariables.ts

@@ -74,29 +110,31 @@ export const getVariables = (figma: PluginAPI, settings: Settings) => {
      // get collection name and modes
      const { variableCollectionId } = variable
      const { name: collection, modes } = collections[variableCollectionId]
+
+      if (modes.length > 1) {


@0m4r, when I comment out this if clause and the content inside it does not break. So, the leak is probably in detectVariableReferencesInCollection.

(you cannot share the Figma file you run your test, right?)

…ollection'

0m4r · 2024-10-11T13:41:17Z

@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

lukasoppermann · 2024-10-25T13:30:59Z

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?

0m4r · 2024-10-25T16:35:48Z

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.

lukasoppermann · 2024-11-01T08:50:08Z

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.

0m4r · 2024-11-01T11:11:06Z

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.

@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?

lukasoppermann · 2024-11-01T11:35:04Z

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

CleanShot.2024-11-01.at.12.32.42.mp4

0m4r · 2024-11-01T11:45:13Z

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.

lukasoppermann · 2024-11-01T11:59:09Z

@0m4r I don't know if it is related to the tokens referencing tokens from another file?

0m4r · 2024-11-01T12:58:53Z

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

0m4r · 2024-11-01T14:18:23Z

@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)

lukasoppermann · 2024-11-13T10:20:57Z

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)
    }

0m4r · 2024-11-13T14:38:14Z

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!

0m4r · 2024-11-18T08:47:25Z

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.

lukasoppermann · 2024-11-18T13:15:55Z

Perfect, thank you for all the work. Sorry that it was such a long process

0m4r mentioned this pull request Jul 5, 2024

Mode name are duplicated in referenced values #308

Closed

Omar Adobati added 2 commits July 8, 2024 08:46

restore accidentally removed image

8ef5cab

improve readability of the Info tooltip element for dark mode

dc9610b

updates

062a14e

miguelalegria approved these changes Jul 29, 2024

View reviewed changes

Merge pull request #1 from lukasoppermann/pr/0m4r/309

ff5fded

Some updates but not working

lukasoppermann closed this Sep 19, 2024

lukasoppermann reopened this Sep 19, 2024

0m4r added 2 commits September 26, 2024 21:46

feat: allow to configure mode names in token name and/or value

a05856e

Add setting configuration to add the mode name to the token name or value. Both token name and value can have the mode added, only one of them or none.

revert manifest name change

2e6419a

0m4r marked this pull request as draft September 26, 2024 20:18

Merge remote-tracking branch 'upstream/main'

6bb59d3

0m4r self-assigned this Sep 27, 2024

0m4r added the bug Something isn't working label Sep 27, 2024

0m4r requested review from lukasoppermann and miguelalegria September 27, 2024 06:36

feat: resolve same mode/collection alias

4998846

0m4r marked this pull request as ready for review September 30, 2024 15:26

0m4r added 2 commits October 8, 2024 08:03

refactor: update label for mode export settings control

dd3c4d7

refactor: update label for mode export settings control

622077e

lukasoppermann reviewed Oct 8, 2024

View reviewed changes

refactor: avoid memory leak generated in 'detectVariableReferencesInC…

96d1f58

…ollection'

0m4r requested a review from lukasoppermann October 19, 2024 11:22

feat: add new setting for 'resolveSameCollectionOrModeReference'

2ee3972

refactor: optimize file data generation for download

3c94f77

0m4r added 2 commits November 18, 2024 09:39

refactor: generate design token JSON only on export command

93d0f25

Merge branch 'main' into main

b6aa348

0m4r removed the request for review from miguelalegria November 18, 2024 12:57

lukasoppermann approved these changes Nov 18, 2024

View reviewed changes

lukasoppermann merged commit 21b152c into lukasoppermann:main Nov 18, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting option to exclude mode from token value #309

Add setting option to exclude mode from token value #309

0m4r commented Jul 5, 2024 •

edited

Loading

0m4r commented Jul 18, 2024

lukasoppermann commented Jul 18, 2024

lukasoppermann commented Jul 19, 2024

0m4r commented Jul 19, 2024

lukasoppermann commented Jul 19, 2024

0m4r commented Jul 22, 2024

lukasoppermann commented Sep 19, 2024

0m4r commented Sep 19, 2024

0m4r commented Sep 26, 2024 •

edited

Loading

coveralls commented Sep 30, 2024 •

edited

Loading

0m4r commented Sep 30, 2024

lukasoppermann commented Oct 7, 2024 •

edited

Loading

0m4r commented Oct 8, 2024

lukasoppermann commented Oct 8, 2024

0m4r commented Oct 8, 2024 •

edited

Loading

lukasoppermann Oct 8, 2024

0m4r Oct 8, 2024

0m4r commented Oct 11, 2024 •

edited

Loading

lukasoppermann commented Oct 25, 2024

0m4r commented Oct 25, 2024

lukasoppermann commented Nov 1, 2024

0m4r commented Nov 1, 2024

lukasoppermann commented Nov 1, 2024

0m4r commented Nov 1, 2024

lukasoppermann commented Nov 1, 2024

0m4r commented Nov 1, 2024

0m4r commented Nov 1, 2024 •

edited

Loading

lukasoppermann commented Nov 13, 2024

0m4r commented Nov 13, 2024

0m4r commented Nov 18, 2024

lukasoppermann commented Nov 18, 2024

Add setting option to exclude mode from token value #309

Add setting option to exclude mode from token value #309

Conversation

0m4r commented Jul 5, 2024 • edited Loading

0m4r commented Jul 18, 2024

lukasoppermann commented Jul 18, 2024

lukasoppermann commented Jul 19, 2024

0m4r commented Jul 19, 2024

lukasoppermann commented Jul 19, 2024

0m4r commented Jul 22, 2024

lukasoppermann commented Sep 19, 2024

0m4r commented Sep 19, 2024

0m4r commented Sep 26, 2024 • edited Loading

coveralls commented Sep 30, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11630838963

Details

💛 - Coveralls

0m4r commented Sep 30, 2024

lukasoppermann commented Oct 7, 2024 • edited Loading

0m4r commented Oct 8, 2024

lukasoppermann commented Oct 8, 2024

0m4r commented Oct 8, 2024 • edited Loading

lukasoppermann Oct 8, 2024

Choose a reason for hiding this comment

0m4r Oct 8, 2024

Choose a reason for hiding this comment

0m4r commented Oct 11, 2024 • edited Loading

lukasoppermann commented Oct 25, 2024

0m4r commented Oct 25, 2024

lukasoppermann commented Nov 1, 2024

0m4r commented Nov 1, 2024

lukasoppermann commented Nov 1, 2024

0m4r commented Nov 1, 2024

lukasoppermann commented Nov 1, 2024

0m4r commented Nov 1, 2024

0m4r commented Nov 1, 2024 • edited Loading

lukasoppermann commented Nov 13, 2024

0m4r commented Nov 13, 2024

0m4r commented Nov 18, 2024

lukasoppermann commented Nov 18, 2024

0m4r commented Jul 5, 2024 •

edited

Loading

0m4r commented Sep 26, 2024 •

edited

Loading

coveralls commented Sep 30, 2024 •

edited

Loading

lukasoppermann commented Oct 7, 2024 •

edited

Loading

0m4r commented Oct 8, 2024 •

edited

Loading

0m4r commented Oct 11, 2024 •

edited

Loading

0m4r commented Nov 1, 2024 •

edited

Loading