tokens-studio / sd-transforms

Custom transforms for Style-Dictionary, to work with Design Tokens that are exported from Tokens Studio
MIT License
186 stars 28 forks source link

[Bug]: alwaysAddFontStyle Affects Font Weight Tokens Incorrectly #298

Open wannesdebacker opened 1 month ago

wannesdebacker commented 1 month ago

What happened?

I've encountered an issue with the alwaysAddFontStyle option in version 1.2.0 that affects fontWeight tokens. AlwaysAddFontStyle should probably only affect typography tokens.

For example we get the following result.

--font-weight-primary-regular-weight: bold;
--font-weight-primary-regular-style: normal;

Reproduction

Configurator Link

Here you see the fontStyle being added unneeded. If you remove alwaysAddFontStyle the tokens are what we expect.

Expected output

--font-weight-primary-regular: 700;

Version

1.2.0

wannesdebacker commented 3 weeks ago

@jorenbroekema can it be that this issue wasn't fully fixed?

For example for this setup: configurator

The italic one is still getting both style and weight.

jorenbroekema commented 2 weeks ago

You mean the secondary regularItalic?

In my PR I did not test for fontweight values that solely have a fontstyle and not a fontweight. I'm actually not entirely sure what the expected output should be in this case, do we just keep it as is:

--font-weight-secondary-regular: 400;
--font-weight-secondary-regular-italic: italic;

Like so?

wannesdebacker commented 2 weeks ago

I think I would expect only:

--font-weight-secondary-regular-italic: 400;

Because the -${style} also doesn't get appended to to the regular version. I would expect them to work the same.

jorenbroekema commented 2 weeks ago

I think I'm still a little confused. Let's isolate it a bit more, reproduction

{
  "foo": {
    "value": "Italic",
    "type": "fontWeights"
  }
}

Expected output, at least in my personal opinion:

:root {
  --foo-weight: 400;
  --foo-style: italic;
}

Because the value "Italic" for type "fontWeights" implies "Regular Italic" because if the fontWeight itself is not specified then it must be the default which is regular, then when that's transformed I expect it to be split up into weight and style and for the weight to be transformed to 400, so the actual output matches my expected output.

Maybe you can elaborate on your expected output and why you expect it

wannesdebacker commented 2 weeks ago

If this happens in a token with type typography it might be logic to do this. But in a type fontWeights I would only expect the foo-weight. And not have the automatic style in place here.

Or I must not fully understand why it's added here.

jorenbroekema commented 2 weeks ago

I get what you're saying but if there's an "italic" there, simply only outputting the (default: regular) weight means we lose that design decision "italic" in the output. How does a developer make sure that the UI that's using these tokens is italic, and in sync with the design, if it's removed from the tokens output?

thomasmattheussen commented 2 weeks ago

I'm confused. Extending your example a bit gives me this result, which is inconsistent.

I guess the way we're using it is through the use of typography tokens.

{
  "fontFamily": {
    "primary": {
      "value": "'Merriweather', 'Merriweather Fallback', serif",
      "type": "fontFamilies"
    }
  },
  "fontWeight": {
    "primary": {
      "regular": {
        "value": "Regular",
        "type": "fontWeights"
      },
      "regularItalic": {
        "value": "Italic",
        "type": "fontWeights"
      },
      "bold": {
        "value": "Bold",
        "type": "fontWeights"
      },
      "boldItalic": {
        "value": "Bold Italic",
        "type": "fontWeights"
      }
    }
  },
  "quote": {
    "blockquote": {
      "typography": {
        "value": {
          "fontFamily": "{fontFamily.primary}",
          "fontWeight": "{fontWeight.primary.boldItalic}",
        },
        "type": "typography"
      }
    }
  }
}

which used to give us this output (which is what we're expecting now, but not getting)

--font-family-primary: 'Merriweather', 'Merriweather Fallback', serif;
--font-weight-primary-bold-italic: 700;
--font-weight-primary-bold: 700;
--font-weight-primary-regular-italic: 400;
--font-weight-primary-regular: 400;
--quote-blockquote-typography-font-style: italic;
--quote-blockquote-typography-font-weight: 700;

don't know if that makes sense to you? This is pretty hard to wrap my head around right now 😅

jorenbroekema commented 2 weeks ago

I'm confused. Extending your example a bit gives me this result, which is inconsistent.

Why is it inconsistent? There's 3 tokens (although 2 of them are smudged into 1, thanks figma) in the input, and there's 3 tokens in the output

which used to give us this output (which is what we're expecting now, but not getting)

It seems like the token names are a bit weird when expanding typography types, I'll need to investigate why it's doing that, ideally you wouldn't get -weight-weight and -weight-style in the token names, but rather just -weight and -style, there might be a technical reason for it to be expanded/named in that way, I'll get back to you on this

thomasmattheussen commented 2 weeks ago

Why is it inconsistent?

Because one of them is expanded into 2 tokens with a different "naming"...

--foo-weight: 700;
--foo-style: italic;

...than the other one that is just put out as 1 token

--bar: 500;

Suppose in some component I have to access these CSS vars

font-style: var(--foo-style);
font-weight: var(--foo-weight);

I can't do the same for the bar token because it's just

font-weight: var(--bar);

that's confusing for people who consume our tokens (and to me, tbh 😅 )

jorenbroekema commented 1 week ago

I get the confusion although I'm not sure how I can improve it given that if you have both the weight and style inside 1 token, I have to expand them into 2 separate tokens, and they cannot both be called "bar", hence "bar-weight" and "bar-style".

jorenbroekema commented 1 week ago

It seems like the token names are a bit weird when expanding typography types, I'll need to investigate why it's doing that, ideally you wouldn't get -weight-weight and -weight-style in the token names, but rather just -weight and -style, there might be a technical reason for it to be expanded/named in that way, I'll get back to you on this

I actually cannot reproduce the issue above anymore, there were some patches and I upgraded configurator recently so perhaps I solved it somewhere in the meantime 🤷🏻‍♂️