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 override order #3138

Merged
merged 4 commits into from
Sep 17, 2024
Merged

fix override order #3138

merged 4 commits into from
Sep 17, 2024

Conversation

six7
Copy link
Collaborator

@six7 six7 commented Sep 14, 2024

Why does this PR exist?

Closes #3139

When we're merging token sets, we weren't ordering token sets as they should. Instead, we were relying on the ordering that existed in the themes. This caused any variable creation or merging of token sets to not respect overrides coming later.

This PR fixes this. In addition, this PR fixes a bug when exporting tokens via Sets - given they both were using that same function I fixed that along the way. Basically, if you exported sets that had math calculations inside, we didnt properly calculate those, as we were only passing in the current set tokens, and not the whole tokens when resolving.

Before

image CleanShot 2024-09-14 at 18 14 36@2x

After

CleanShot 2024-09-14 at 17 59 38@2x CleanShot 2024-09-14 at 18 15 02@2x

Testing this change

Load the .JSON linked in the Issue (single file import). Try to create before and after.

Additional Notes (if any)

Copy link

changeset-bot bot commented Sep 14, 2024

🦋 Changeset detected

Latest commit: 216f31f

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 14, 2024

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

@@ -5,6 +5,6 @@
}
variable.setValueForMode(mode, value);
} catch (e) {
console.error('Error setting numberVariable', e);
console.error('Error setting numberVariable on variable', variable.name, e);

Check warning

Code scanning / ESLint

disallow the use of `console` Warning

Unexpected console statement.
Copy link
Contributor

github-actions bot commented Sep 14, 2024

Commit SHA:4abe20174c6d4490c43575f712f91ee45506fd3b

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: fix/override-order 
Status File % Stmts % Branch % Funcs % Lines
🟢 total 67.85 (0.07) 58.51 (0.09) 64.57 (0.04) 68.25 (0.04)
🔴 packages/tokens-studio-for-figma/src/plugin/createLocalVariablesInPlugin.ts 53.12 (-1.71) 38.09 (0) 66.66 (0) 53.33 (-1.84)
🟢 packages/tokens-studio-for-figma/src/plugin/createLocalVariablesWithoutModesInPlugin.ts 36.84 (5.03) 35 (0) 20 (5.72) 38.88 (4.74)
🟢 packages/tokens-studio-for-figma/src/plugin/generateTokensToCreate.ts 100 (0) 85.71 (2.38) 100 (0) 100 (0)
✨ 🆕 packages/tokens-studio-for-figma/src/utils/getTokenSetsOrder.ts 100 100 100 100
✨ 🆕 packages/tokens-studio-for-figma/src/utils/sortSets.ts 100 100 100 100
🔴 packages/tokens-studio-for-figma/src/utils/tokenHelpers.ts 53.33 (-7.78) 57.69 (-3.67) 41.66 (-17.16) 60 (-9.04)

Copy link
Contributor

github-actions bot commented Sep 14, 2024

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

@six7 six7 marked this pull request as ready for review September 14, 2024 16:15
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 f6398c7 into main Sep 17, 2024
10 of 11 checks passed
@six7 six7 deleted the fix/override-order branch September 17, 2024 09:51
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.

Set overrides are not being respected
2 participants