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 typography tokens resolution #3157

Merged
merged 8 commits into from
Oct 1, 2024

Conversation

akshay-gupta7
Copy link
Contributor

@akshay-gupta7 akshay-gupta7 commented Sep 26, 2024

Addresses #3118

Description

Fixes a bug occurring when a user applies a typography token on a text node, and then applies another token, tinkering with an individual typography property(like font size or font family, etc.), and the user clicks on 'Apply to selection', the typography token overrides the individual token property, which is not expected behaviour.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • None of the above

How to test this

  • Create a typography token with references
  • Apply the token to a text
  • Apply a any token overriding one of the typography properties(like font size, or font family)
  • Then Apply to selection
  • The individual token typography should be applied and work as expected

Screenshots or video (if necessary):

Copy link

changeset-bot bot commented Sep 26, 2024

🦋 Changeset detected

Latest commit: b946902

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

@akshay-gupta7 akshay-gupta7 self-assigned this Sep 26, 2024
Copy link
Contributor

github-actions bot commented Sep 26, 2024

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

@akshay-gupta7 akshay-gupta7 linked an issue Sep 26, 2024 that may be closed by this pull request
@akshay-gupta7 akshay-gupta7 marked this pull request as draft September 26, 2024 14:24
Copy link
Contributor

github-actions bot commented Oct 1, 2024

Commit SHA:c279d076dd37af344d438226f4b8768277ff5e34

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: fix-typography-tokens-resolution 
Status File % Stmts % Branch % Funcs % Lines
🟢 total 67.9 (0) 58.51 (0) 64.65 (0.02) 68.29 (0)
🟢 packages/tokens-studio-for-figma/src/plugin/applyTypographyTokenOnNode.ts 78.26 (2.07) 53.24 (-0.41) 100 (0) 76.19 (1.19)

Comment on lines +268 to 269
expect(setTextValuesOnTargetSpy).not.toHaveBeenCalled();
});
Copy link
Contributor Author

@akshay-gupta7 akshay-gupta7 Oct 1, 2024

Choose a reason for hiding this comment

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

This update in condition is in line with the updated logic, wherein, when a text node has both individual typography value tokens and a typography token, it will not call setTextValuesOnTarget directly, instead will route to tryApplyTypographyCompositeVariable.

@akshay-gupta7 akshay-gupta7 marked this pull request as ready for review October 1, 2024 09:59

// Apply typography token directly if no other properties exist
if (data.typography && resolvedToken && isSingleTypographyValue(resolvedToken.value) && !Object.keys(values).length) {
setTextValuesOnTarget(node, data.typography, baseFontSize);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now we will call this setTextValuesOnTarget here when a text Node has only a typography token applied to it

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

@akshay-gupta7 akshay-gupta7 merged commit a6eb07d into main Oct 1, 2024
9 checks passed
@akshay-gupta7 akshay-gupta7 deleted the fix-typography-tokens-resolution branch October 1, 2024 18:32
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.

Typography Composite Token not being Overridden
2 participants