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 themes: include fallback logic #3111

Merged
merged 16 commits into from
Sep 10, 2024
Merged

fix themes: include fallback logic #3111

merged 16 commits into from
Sep 10, 2024

Conversation

six7
Copy link
Collaborator

@six7 six7 commented Sep 2, 2024

This addresses a flaw introduced in 2.0.2

Fixed #3110

Before 2.0.2 we looked at all theme configurations an created styles based on the combination of them all, which wasn't always creating expected results.

In 2.0.2 we changed that to only look at the active theme's selectedSets, but that meant users who didnt have their references created properly, got incorrectly created styles

This PR changes things to have the following logic:

  • We combine all theme's selectedSets into an overallConfig which acts as the baseline, the fallback.
  • For each theme, we rank the current theme's selectedSets higher, so they will always win
  • When creating styles, we combine those
  • In addition, we add all sets to the list, with a very low ranking. This effectively means, we run WAY less into issues where we are missing

Also, it fixes the "Remove styles" problem which was always deleting styles for users when they're not part of the current theme. Now, we look at the overall created styles and then remove those that are not touched, instead of doing so for each theme which is always wrong.

In addition, I'm disabling token-transformer tests. We're not maintaining this anymore, and this changes core logic on the transformer, which doesnt make sense to keep testing now that we have Style Dictionary.

Copy link

changeset-bot bot commented Sep 2, 2024

🦋 Changeset detected

Latest commit: 457fe32

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 Minor

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

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

@rbosker
Copy link
Collaborator

rbosker commented Sep 2, 2024

tested this build vs the current prod version:

  • Prod version doesn't export styles if the 'core' is disabled and can't resolve the colors. If I have that set as active or source, it does work.
  • This build works for all those 3 cases, also when 'core' is disabled.

Tested with (I think) all combinations of export settings.

@rbosker
Copy link
Collaborator

rbosker commented Sep 2, 2024

@six7 One issue I found: the loader stays indicating 'Create styles', but styles are created.

Screen.Recording.2024-09-02.at.12.47.59.mov

Copy link
Contributor

github-actions bot commented Sep 6, 2024

Commit SHA:2b58a6f8248151392682110695962105783ae86e

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: six7/themes-refactor 
Status File % Stmts % Branch % Funcs % Lines
🔴 total 67.78 (-0.05) 58.41 (-0.06) 64.5 (0) 68.2 (-0.03)
🟢 packages/tokens-studio-for-figma/src/app/store/useTokens.tsx 52.74 (0.85) 34.61 (0.77) 56.25 (2.52) 53.6 (0.88)
🔴 packages/tokens-studio-for-figma/src/utils/tokenHelpers.ts 61.11 (-29.36) 61.36 (-33.87) 58.82 (-16.18) 69.04 (-25.69)

Copy link
Contributor

github-actions bot commented Sep 6, 2024

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

@six7 six7 marked this pull request as ready for review September 7, 2024 08:17
@akshay-gupta7 akshay-gupta7 self-requested a review September 10, 2024 15:02
@six7 six7 merged commit 0bc599e into main Sep 10, 2024
10 of 11 checks passed
@six7 six7 deleted the six7/themes-refactor branch September 10, 2024 15: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.

Issue creating styles when themes are not created _exactly_ as needed
3 participants