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 variable creation issue #3124

Merged
merged 5 commits into from
Sep 10, 2024
Merged

Fix variable creation issue #3124

merged 5 commits into from
Sep 10, 2024

Conversation

six7
Copy link
Collaborator

@six7 six7 commented Sep 5, 2024

This pull request solves 2 issues:

Font weight issues

When users were using numerical font weights in their typography tokens, and they had a varaible reference to both the family and the numerical weight, we didnt properly create styles. This only occurred if the font family and weight wasnt covered by our font weight fallbacks (e.g. use Comic Sans MS with a weight of 600)
This is due to Figma requiring us to loadFontAsync using the string version of the weight ("Bold"). However, all we know is the numerical weight ("600"). We can not realistically know the string version. So this PR changes this to rather load all font weights of the current font family.

Color token issues with modifiers when creating color styles

If users were using a color token that had a pure reference such as {colors.gray.500} and a modifier, and the colors.gray.500 token existed as a variable, previously we would use a variable reference. We now check if the token is using a modifier, and if yes, we do not go an create a variable reference.
This issue ONLY occurred if the Variable exists as a reference and the user is trying to create color styles using those variable reference

CleanShot.2024-09-06.at.09.22.39.mp4

https://github.com/user-attachments/assets/66ef0c75-e144-4113-9c47-66c8186e6b91
example.json

Copy link

changeset-bot bot commented Sep 5, 2024

🦋 Changeset detected

Latest commit: b538dc5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@tokens-studio/figma-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Sep 5, 2024

⤵️ 📦 ✨ The artifact was successfully created! Want to test it? Download it here 👀 🎁

successfullyAppliedVariable = true;
} catch (e) {
// eslint-disable-next-line no-console
console.error('unable to apply variable', key, variableToApply, e);

Check warning

Code scanning / ESLint

disallow the use of `console` Warning

Unexpected console statement.
Copy link
Contributor

github-actions bot commented Sep 6, 2024

Commit SHA:c64bfb16c45192973694d102cf2e6d1ce303d2be

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: six7/fix-variable-creation-issue 
Status File % Stmts % Branch % Funcs % Lines
🟢 total 67.78 (0) 58.42 (0.01) 64.53 (0.03) 68.21 (0.01)
🟢 packages/tokens-studio-for-figma/src/plugin/setColorValuesOnTarget.ts 92.06 (0.13) 87.23 (0.14) 100 (0) 94.54 (0.1)
🔴 packages/tokens-studio-for-figma/src/plugin/setFontStyleOnTarget.ts 93.75 (0) 72.72 (-4.55) 100 (0) 96.66 (0)
🔴 packages/tokens-studio-for-figma/src/plugin/tryApplyTypographyCompositeVariable.ts 87.17 (-5.93) 80 (-0.64) 100 (0) 90.9 (-9.1)

Copy link
Contributor

github-actions bot commented Sep 6, 2024

Commit SHA:c64bfb16c45192973694d102cf2e6d1ce303d2be
Current PR reduces the test coverage percentage by 1 for some tests

@six7 six7 marked this pull request as ready for review September 6, 2024 07:10
Copy link
Contributor

@macintoshhelper macintoshhelper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@six7 six7 merged commit 087b4c1 into main Sep 10, 2024
10 of 11 checks passed
@six7 six7 deleted the six7/fix-variable-creation-issue branch September 10, 2024 16:09
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.

3 participants