-
Notifications
You must be signed in to change notification settings - Fork 29
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
[Bug]: alwaysAddFontStyle Affects Font Weight Tokens Incorrectly #298
Comments
@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. |
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? |
I think I would expect only:
Because the |
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 |
If this happens in a token with type Or I must not fully understand why it's added here. |
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? |
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.
which used to give us this output (which is what we're expecting now, but not getting)
don't know if that makes sense to you? This is pretty hard to wrap my head around right now 😅 |
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
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 |
Because one of them is expanded into 2 tokens with a different "naming"...
...than the other one that is just put out as 1 token
Suppose in some component I have to access these CSS vars
I can't do the same for the
that's confusing for people who consume our tokens (and to me, tbh 😅 ) |
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". |
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 🤷🏻♂️ |
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.
Reproduction
Configurator Link
Here you see the
fontStyle
being added unneeded. If you removealwaysAddFontStyle
the tokens are what we expect.Expected output
--font-weight-primary-regular: 700;
Version
1.2.0
The text was updated successfully, but these errors were encountered: