Fix import issue #3270
Fix import issue #3270
Conversation
🦋 Changeset detectedLatest commit: 7ef0d00 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
|
|
Commit SHA:188795cd7456f505bb1a779ecfacf6b78a432c1c Test coverage results 🧪
|
|
Commit SHA:188795cd7456f505bb1a779ecfacf6b78a432c1c |
There was a problem hiding this comment.
PR Overview
This PR fixes an import issue and extends the functionality of the variable pull process to create theme groups and options for pro users. The changes update type imports, function signatures, and tests to accommodate new parameters and update the notifier and UI state accordingly.
- Updated pullVariables to accept themes and proUser parameters and process theme groups.
- Adjusted async message handlers, notifiers, tests, and UI components to support the new theme functionality.
- Updated types for async messages and plugin messages to include themes and proUser.
Changes
| File | Description |
|---|---|
| packages/tokens-studio-for-figma/src/plugin/pullVariables.ts | Updated function signature, theme processing logic, and async handling for variable collections. |
| packages/tokens-studio-for-figma/src/plugin/pullVariables.test.ts | Updated tests to pass themes and proUser parameters. |
| packages/tokens-studio-for-figma/src/plugin/asyncMessageHandlers/pullVariables.ts | Updated to pass new parameters to pullVariables. |
| packages/tokens-studio-for-figma/src/plugin/notifiers.ts | Modified notifyVariableValues to support themes. |
| packages/tokens-studio-for-figma/src/app/components/Initiator.tsx | Updated to dispatch themes received from plugin messages. |
| packages/tokens-studio-for-figma/src/app/store/useTokens.tsx | Updated to derive proUser from license keys and pass themes/proUser to pullVariables. |
| packages/tokens-studio-for-figma/src/types/AsyncMessages.ts | Updated PullVariablesAsyncMessage to include themes and proUser. |
| packages/tokens-studio-for-figma/src/types/messages.tsx | Added themes field to the VariablesFromPluginMessage type. |
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| @@ -68,6 +70,9 @@ export default function useTokens() { | |||
| const store = useStore<RootState>(); | |||
| const tokensContext = useContext(TokensContext); | |||
| const shouldConfirm = useMemo(() => updateMode === UpdateMode.DOCUMENT, [updateMode]); | |||
| const existingKey = useSelector(licenseKeySelector); | |||
| const licenseKeyError = useSelector(licenseKeyErrorSelector); | |||
| const proUser = Boolean(existingKey && !licenseKeyError); | |||
There was a problem hiding this comment.
i think we should have a prouser property in uiState somewhere - i might be wrong though. basically the one that we use to display the GET PRO badge or the PRO badge - is that this one?
| }>(); | ||
|
|
||
| localVariables.forEach(async (variable) => { | ||
| const collection = figma.variables.getVariableCollectionById(variable.variableCollectionId); |
There was a problem hiding this comment.
is this a slow task? should we try to cache this? also.. this is deprecated according to the figma plugin api and we should use the async version: https://www.figma.com/plugin-docs/api/properties/figma-variables-getvariablecollectionbyid/
| // Process themes if pro user | ||
| const updatedThemes = [...themes]; | ||
| if (proUser) { | ||
| collections.forEach((collection) => { |
There was a problem hiding this comment.
can we also instead of doing a forEach do a promise.all to have them run all at the same time?
| if (!modeExists) { | ||
| updatedThemes.push({ | ||
| id: `${collection.name.toLowerCase()}-${mode.name.toLowerCase()}`, | ||
| name: mode.name, | ||
| group: collection.name, | ||
| selectedTokenSets: { | ||
| [`${collection.name}/${mode.name}`]: TokenSetStatus.ENABLED, | ||
| }, | ||
| $figmaStyleReferences: {}, | ||
| $figmaModeId: mode.modeId, | ||
| $figmaCollectionId: collection.id, | ||
| }); | ||
| } | ||
| } else { | ||
| updatedThemes.push({ | ||
| id: `${collection.name.toLowerCase()}-${mode.name.toLowerCase()}`, | ||
| name: mode.name, | ||
| group: collection.name, | ||
| selectedTokenSets: { | ||
| [`${collection.name}/${mode.name}`]: TokenSetStatus.ENABLED, | ||
| }, | ||
| $figmaStyleReferences: {}, | ||
| $figmaModeId: mode.modeId, | ||
| $figmaCollectionId: collection.id, | ||
| }); |
There was a problem hiding this comment.
i'm having trouble finding the difference between these two 🤔 do we need the condition if they do the same thing?
There was a problem hiding this comment.
yeah, I updated the logic a bit, to fix a bug for the variables being imported as themes even before clicking 'Import All'
Why does this PR exist?
Fixes #3260
What does this pull request do?
Currently, when users import variables, the plugin only creates corresponding sets for collections and variables.
This PR adds the feature to create theme groups as well, containing the corresponding sets created as well.
Testing this change
Additional Notes (if any)