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

fix aliased variables being emmited twice #289

Closed
wants to merge 2 commits into from
Closed

fix aliased variables being emmited twice #289

wants to merge 2 commits into from

Conversation

MonsieurMan
Copy link

Hi there, this is just a quick fix for a bug I had.

Aliased tokens with different modes were emitted twice (with two modes) as there were two place in the code where modes were taken into account.

This removes the loop in the processAliasModes fn, just keeping L98 which is already looping over modes.

I didn't tested it with more than two modes for now.


Thanks for the plugin, I'm doing research for implementing a multi-brand design system and I'll maybe use it, could contribute more if it is the case and you're open to it.

@lukasoppermann
Copy link
Owner

Hey @MonsieurMan this makes sense.

Could you explain a way to test this to me?

How to I reproduce the issue with double variables? What setup do I need to create this? Afterward I can test this.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6722612248

  • 1 of 6 (16.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 66.667%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/utilities/getVariables.ts 1 6 16.67%
Totals Coverage Status
Change from base Build 6708682924: 0.2%
Covered Lines: 496
Relevant Lines: 728

💛 - Coveralls

@JackHowa
Copy link
Contributor

I'm having a similar issue. And I'm not sure if it's related to #293 where the last theme seems to be being the default mode for aliased tokens. I think I can write a new test file under tests/ folder

@lukasoppermann
Copy link
Owner

@JackHowa this would be great. All we need is a way to test this and than we can merge it.

@JackHowa
Copy link
Contributor

JackHowa commented Jan 10, 2024

@JackHowa this would be great. All we need is a way to test this and than we can merge it.

Cool, working on it now with src/utilities/getVariables.ts. Investigating what figma gives you in terms of data to generate something like this mode:

  "colors": {
    "themeA": {
      "palette": {
        "brand": {
          "primary": {
            "type": "color",
            "value": "#001944ff",
            "blendMode": "normal"
          }
        }
      },
      "surfaces": {
        "primary": {
          "default": {
            "type": "color",
            "value": "{colors.themeC.palette.brand.secondary}"
          }
        }
      }
    },
    "themeB": {
      "palette": {
        "brand": {
          "primary": {
            "type": "color",
            "value": "#001944ff",
            "blendMode": "normal"
          }
        }
      },
      "surfaces": {
        "primary": {
          "default": {
            "type": "color",
            "value": "{colors.themeC.palette.brand.secondary}"
          }
        }
      }
    },
    "themeC": {
      "palette": {
        "brand": {
          "primary": {
            "type": "color",
            "value": "#001944ff",
            "blendMode": "normal"
          }
        }
      },
      "surfaces": {
        "primary": {
          "default": {
            "type": "color",
            "value": "{colors.themeC.palette.brand.secondary}"
          }
        }
      }
    }
  }

@JackHowa
Copy link
Contributor

This works as expected for me locally. I will work on trying to unit test extractTokens. But will involve mocking figma's api

@JackHowa
Copy link
Contributor

JackHowa commented Jan 18, 2024

@lukasoppermann @MonsieurMan thanks for your work on this and patience. I have added some tests in a separate pr off the default that shows the alias modes emitting two values to show https://github.com/lukasoppermann/design-tokens/pull/299/files#diff-18b59c301a30dbea8cef574b8e83d81e812fa126a021df0860ff518f3dd21035R17

I'm happy to make a pr off of this branch if that's helpful. Like I said before, this is working for our usecase! Thanks!

@lukasoppermann
Copy link
Owner

@JackHowa can you please resolve conflicts? I think you know better what is correct.

@JackHowa
Copy link
Contributor

JackHowa commented Jun 26, 2024

@JackHowa can you please resolve conflicts? I think you know better what is correct.

sorry, this isn't my pr. looks like my pr was just merged! #300 🥳 Thanks so much @lukasoppermann !!

@lukasoppermann
Copy link
Owner

@JackHowa sorry, I completely missed this the whole time. 🤦

@JackHowa
Copy link
Contributor

@JackHowa sorry, I completely missed this the whole time. 🤦

hahah no worries at all. thanks for the quick replies. your plugin is a lifesaver for my org

@MonsieurMan MonsieurMan closed this by deleting the head repository Jun 27, 2024
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.

4 participants