mikaelvesavuori / figmagic

Figmagic is the missing piece between DevOps and design: Generate design tokens, export graphics, and extract design token-driven React components from your Figma documents.
https://docs.figmagic.com
MIT License
807 stars 71 forks source link

Variants only working correctly with duplicate layers #165

Closed davidbwaters closed 2 years ago

davidbwaters commented 2 years ago

Describe the bug Hi, element variants only work right for me for text styles if I have 2 variants of the same name. Otherwise duplicate styles for font properties are applied to the wrong class.

To Reproduce Steps to reproduce the behavior: using this figma file. https://www.figma.com/file/jntgBgYNVBnx1PfSBhbqGr/MTX-Figmagic?node-id=3982%3A22235

Expected behavior Variant styles should generate correctly.

Additional context I've had similar issues with variants acting funny like the border color for buttons not working correctly, but this is more major.

davidbwaters commented 2 years ago

Actually, the weights, sizes, and families are still wrong with the generation here.

CleanShot 2022-08-04 at 17 58 04@2x

mikaelvesavuori commented 2 years ago

Each element is mapped to one Figma component. In your Figma file you have them grouped under a component, so this will not work. I appreciate that this will create a bit of redundancy in your case such as with the Button, but that's just the way it has to work to be multi-functional, as we cannot otherwise infer the desired use. So in short: You will need to make individual components for each Button variety. The generated CSS looks fine, though is of course nested wrongly here, based on what I just wrote.

davidbwaters commented 2 years ago

I thought this was the point of nested vs flat elements, to group variants under a single component? Buttons work okay, with some tweaking. It's the text component I'm referring to.

mikaelvesavuori commented 2 years ago

I can see that being misunderstood for sure! Unfortunately what you are writing is not the full truth: The nesting pertains only to accepting subgroups (or now also subframes) for styling purposes. This is not done with ”flat” elements.

You should be able to see this subtle difference in doing this in the template (https://www.figma.com/community/file/821094451476848226) vs in your file.

I will see if it makes sense updating docs to be better at explaining this.

mikaelvesavuori commented 2 years ago

As far as I can tell from your Figma file, you have the same type of issue with your Text Figma component, and how it is incorrectly using the expected pattern (i.e. you are putting them as nested groups in one large component). Solution: You should have each text layer as its own component.

davidbwaters commented 2 years ago

So in your example, the color of the button can be a different variant, but not something like font properties, basically?

mikaelvesavuori commented 2 years ago

Well it should theoretically be able to have whatever properties you want, though in my example (as in most conventional cases) they only have marginal differences.

For better or worse, as soon as they are their own components they are effectively unique from each other. That's why the nesting comes in to aid with visual or minor variations.

davidbwaters commented 2 years ago

Yeah, I'm now more confused as far as the nested use case? Maybe the terms "variants" in the docs makes it kinda confusing, just not sure why it's outputting redundant font properties like that with the text element. The figma layout is the same for all of the subgroups, the only difference is their properties.

mikaelvesavuori commented 2 years ago

Setting up some type of demo on my end. Until then, did you see and read https://github.com/mikaelvesavuori/figmagic/blob/main/readme/elements.md? EDIT: I am assuming you did, given some of the questions... Will update the docs to never say the word "variant" btw.

davidbwaters commented 2 years ago

Thanks a lot. Appreciate the awesome support on this project.

mikaelvesavuori commented 2 years ago

OK... (EDIT: fixed code + image of implementation)

Here's how the canonical way of implementing a TextTinier element would look.

Screenshot 2022-08-05 at 13 40 59

That should be clear enough on how nested elements work. As you are on to, a future evolution might even be able to squish both "flat" and "nested" into the same implementation, making it easier to grok (now we are at the deep end of legacy in this project!).

However I would discourage this specific use and think of only Text or Paragraph or something, rather than the very broad range of elements you've made. This is just to keep it DRY in code and opt for a multi-purpose component rather than uniques for all individual styles. To do the styling you would then have to drive it with props instead. See for example https://github.com/mikaelvesavuori/figmagic-example

To be fair, this is where conventional coding wisdom outsmarts any generated code; my intent was never to fully disregard custom coding but to speed it up by scaffolding as much as possible and then doing the necessary tweaks.

Next up: Font families.

const TextTinierCss = `
  color: ${colors['white']};
  font-size: ${fontSizes['tinier']};
  font-family: ${fontFamilies['heading']};
  font-family: ${fontFamilies['strong']};
  font-family: ${fontFamilies['normal']};
  font-family: ${fontFamilies['light']};
  line-height: ${lineHeights['normal']};
  text-align: left;

  &.Standard {
    font-weight: ${fontWeights['normal']};
  }

  &.Strong {
    font-weight: ${fontWeights['strong']};
    font-weight: ${fontWeights['light']};
  }

`;

(For the life of me, however, I don't understand why the "Light" style is not showing up)

The generated code looks like the above, and now I was reminded of a behavior that's not really a bug, but which is a pain in the ass. The multiple font-family properties are technically correct, since it does match all those font families in your design token page.

Screenshot 2022-08-05 at 13 21 43

The secret here, and it should be clear when you start grasping that tokens are strictly uni-dimensional, meaning that regardless of how much you decorate and do with a "thing" in the design tokens page frames, only a single property will be exported as a design token (e.g. the property referred to in the frame name). Ergo, in Font Families, you've added several "features/properties" to all of them. Example: Heading - 500 140%. The proper use for you would be to create a single family token, for example named GT America. Subsequently, your exported element would no longer use multiple matches, but only the one. You can read more about uni-dimensionality at https://github.com/mikaelvesavuori/figmagic#unidimensional-or-multidimensional-values.

This is the specific reason the template file uses a "Fonts" (un-exported) frame to communicate the design but not mess up the wiring between Figma and Figmagic.

davidbwaters commented 2 years ago

OK... (EDIT: fixed code + image of implementation)

Here's how the canonical way of implementing a TextTinier element would look.

Screenshot 2022-08-05 at 13 40 59

That should be clear enough on how nested elements work. As you are on to, a future evolution might even be able to squish both "flat" and "nested" into the same implementation, making it easier to grok (now we are at the deep end of legacy in this project!).

However I would discourage this specific use and think of only Text or Paragraph or something, rather than the very broad range of elements you've made. This is just to keep it DRY in code and opt for a multi-purpose component rather than uniques for all individual styles. To do the styling you would then have to drive it with props instead. See for example https://github.com/mikaelvesavuori/figmagic-example

To be fair, this is where conventional coding wisdom outsmarts any generated code; my intent was never to fully disregard custom coding but to speed it up by scaffolding as much as possible and then doing the necessary tweaks.

Next up: Font families.

const TextTinierCss = `
  color: ${colors['white']};
  font-size: ${fontSizes['tinier']};
  font-family: ${fontFamilies['heading']};
  font-family: ${fontFamilies['strong']};
  font-family: ${fontFamilies['normal']};
  font-family: ${fontFamilies['light']};
  line-height: ${lineHeights['normal']};
  text-align: left;

  &.Standard {
    font-weight: ${fontWeights['normal']};
  }

  &.Strong {
    font-weight: ${fontWeights['strong']};
    font-weight: ${fontWeights['light']};
  }

`;

(For the life of me, however, I don't understand why the "Light" style is not showing up)

The generated code looks like the above, and now I was reminded of a behavior that's not really a bug, but which is a pain in the ass. The multiple font-family properties are technically correct, since it does match all those font families in your design token page.

Screenshot 2022-08-05 at 13 21 43

The secret here, and it should be clear when you start grasping that tokens are strictly uni-dimensional, meaning that regardless of how much you decorate and do with a "thing" in the design tokens page frames, only a single property will be exported as a design token (e.g. the property referred to in the frame name). Ergo, in Font Families, you've added several "features/properties" to all of them. Example: Heading - 500 140%. The proper use for you would be to create a single family token, for example named GT America. Subsequently, your exported element would no longer use multiple matches, but only the one. You can read more about uni-dimensionality at https://github.com/mikaelvesavuori/figmagic#unidimensional-or-multidimensional-values.

This is the specific reason the template file uses a "Fonts" (un-exported) frame to communicate the design but not mess up the wiring between Figma and Figmagic.

Okay, yeah I'm actually using postscript font names, so the output is actually different families for weights. I had to do this because the wide/expanded find family was the same as the others and needed a way to make it export correctly. Maybe that's causing the issue, I didn't think about that.

Also, the font-weight: light, under the strong style would override the weight, I think.

All the online figma token services that parse files have had issues with the expanded font being the same family and figmagic using postscript naming is the only way I can get it to make the distinction.

mikaelvesavuori commented 2 years ago

So @davidbwaters, I feel like we've come some ways on this topic, but a lot of it has to do with user-side implementation "issues". So if you don't mind I'd rather close this and we can maybe have some other conversation or so, in case there is something specific I can support with?

davidbwaters commented 2 years ago

Yeah, that's fine. Still not sure why the duplicate properties (like weight in the last example) are added to the wrong variants.

mikaelvesavuori commented 2 years ago

So what's become apparent is that the flat and nested elements code bases are pretty dissimilar and I'm reminded of how they really should be synced. I am attempting to bring them closer to each other during the weekend. What you are seeing is that the deduplication done for nested elements is not done for your texts, meaning you end up with duplicate properties (prior dialog is still valid for certain cases, I believe).

mikaelvesavuori commented 2 years ago

I've released 4.5.2 that includes a range of small fixes, including forcing the getUniqueValues function to only use the first occurrence of a property (previously it matched the full line: key + value).

See https://github.com/mikaelvesavuori/figmagic/releases/tag/v4.5.2

davidbwaters commented 2 years ago

Awesome. Thanks, I'll try it

davidbwaters commented 2 years ago

Yeah, I'm still getting the same issues with the text element. Duplicate values and not generating all the classes for the variants. Thanks for the progress, though.

davidbwaters commented 2 years ago

Sorry, i retested and it's working much better, thanks