-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat(exporter): don't export figma-only variables #65
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this filter is working - when I re-exported vars from figma for #63, it pulled down some "figma-only" tokens, and after manually adding the code from this PR to filter those out, those tokens are no longer in the export list.
However, we have some new typography tokens that have "px" appended that don't need them and some of the CSS declarations aren't including the var()
declaration properly. Should that be a separate issue @srambach or want to try and address it in this PR?
// typography tokens with "px" that isn't needed
--pf-t--global--font--line-height--200: 1.5px;
--pf-t--global--font--line-height--100: 1.2999999523162842px;
--pf-t--global--font--family--300: Red Hat Mono VFpx;
--pf-t--global--font--family--200: Red Hat Display VFpx;
--pf-t--global--font--family--100: Red Hat Text VFpx;
// font weight tokens have incorrect value
--pf-t--global--font--line-height--heading: var(--pf-t--global--font--line-height--200);
--pf-t--global--font--line-height--body: var(--pf-t--global--font--line-height--100);
--pf-t--global--font--weight--heading: {global.font.weight.heading.100};
--pf-t--global--font--weight--body: {global.font.weight.body.100};
--pf-t--global--font--family--mono: var(--pf-t--global--font--family--300);
--pf-t--global--font--family--heading: var(--pf-t--global--font--family--200);
--pf-t--global--font--family--body: var(--pf-t--global--font--family--100);
After experimenting, it looks like the transform isn't matching
|
i'd be fine renaming to font--weight--[100-400] and removing the body/heading in the base tokens. |
Cool - ultimately we should probably understand why it doesn't handle it, but I think this particular change makes sense for the base tokens. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎉 This PR is included in version 1.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #61
This will ignore variables that include "figma-only"
To test, use a branch of the tokens figma file that has figma-only files and run the exporter. Check that there are no tokens with "figma-only" in the name and that all others are still there.