allow for bulk remap with variable and styles #2405

robinhoodie0823 · 2023-12-04T03:35:13Z

No description provided.

changeset-bot · 2023-12-04T03:35:17Z

🦋 Changeset detected

Latest commit: c376148

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

github-actions · 2023-12-04T03:36:41Z

PR Analysis

🎯 Main theme: The main theme of this PR is to allow bulk remapping with variables and styles in the application.
📝 PR summary: This PR introduces the ability to perform bulk remapping with variables and styles. It includes changes in several files to handle the new functionality. The changes involve adding new variables and styles, updating the NodeManager class, and modifying the bulkRemapTokens function.
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
⏱️ Estimated effort to review [1-5]: 4, because the PR involves changes in multiple files and introduces new functionality, which requires a good understanding of the existing codebase to review effectively.
🔒 Security concerns: No security concerns found

PR Feedback

💡 General suggestions: The PR seems to be well structured and the changes are logically grouped. However, it would be beneficial to add relevant tests to ensure the new functionality works as expected and does not introduce any regressions.
🤖 Code feedback:
- relevant file: src/plugin/asyncMessageHandlers/bulkRemapTokens.ts
  suggestion: It seems like there's a lot of repeated code in the new additions for handling variables and styles. Consider creating a helper function to handle the shared logic. [important]
  relevant line: const newValue = JSON.parse(savedStyleData).replace(oldName, newName);
- relevant file: src/plugin/SharedDataHandler.ts
  suggestion: It's not clear why an empty line was added after the result declaration. If it's not necessary, consider removing it to maintain code cleanliness. [medium]
  relevant line:
- relevant file: src/plugin/updateNodes.ts
  suggestion: The newTokenValues variable seems to be a combination of appliedTokens and variables. It would be helpful to add a comment explaining why these are being combined. [medium]
  relevant line: const newTokenValues = { ...appliedTokens, ...variables };
- relevant file: src/plugin/pluginData.ts
  suggestion: The ternary operator used in the value assignment is quite long and may be hard to read. Consider breaking it down into multiple lines or extracting the logic into a separate function for better readability. [medium]
  relevant line: value: curr.node.getSharedPluginData(SharedPluginDataNamespaces.VARIABLES, variable.type).length === 0 ? variable.name : JSON.parse(curr.node.getSharedPluginData(SharedPluginDataNamespaces.VARIABLES, variable.type)),

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

github-actions · 2023-12-04T03:40:58Z

Commit SHA:8590c62eac168d3e1e3c794a9971027ad57cf333

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: 2120-allow-for-bulk-remapping-with-variables-styles

Status	File	% Stmts	% Branch	% Funcs	% Lines
🔴	total	70.45 (-0.23)	60.68 (-0.28)	68.61 (-0.26)	70.71 (-0.22)
🔴	src/plugin/asyncMessageHandlers/bulkRemapTokens.ts	45.2 (-50.03)	9.09 (-90.91)	28.57 (-71.43)	47.14 (-48.09)

github-actions · 2023-12-04T03:40:59Z

Commit SHA:8590c62eac168d3e1e3c794a9971027ad57cf333
Current PR reduces the test coverage percentage by 100 for some tests

.changeset/unlucky-forks-mate.md

src/app/components/InspectorTokenSingle.tsx

six7 · 2023-12-05T21:34:15Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

+            const savedVariableData = node.getSharedPluginData(SharedPluginDataNamespaces.VARIABLES, variableType);
+            if (savedVariableData.length > 0) {
+              if (savedVariableData.includes(oldName)) {
+                const newValue = JSON.parse(savedVariableData).replace(oldName, newName);
+                const jsonValue = JSON.stringify(newValue);
+                node.setSharedPluginData(SharedPluginDataNamespaces.VARIABLES, variableType, jsonValue);


I think this is a misunderstanding. We should not get or set variables on shared plugin data. Variables can be set directly using boundVariables and the other variable functions. Here you've applied shared plugin data for variables instead.

There's no need to do this. Let's just read what variables are applied on the layer (Inspect already shows this!), then take the names of those, remap parts of the name, and then find the connected variables to replace it with.

For example, if a variable named colors.blue.500 is applied, and another layer has colors.blue.400, and I remap 'blue' to 'red' then we'd need to

look up tokens named 'colors.red.400'

and 500 and fetch the connected variables

replace the applied variables with those

six7 · 2023-12-05T21:34:34Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

+                const jsonValue = JSON.stringify(newValue);
+                node.setSharedPluginData(SharedPluginDataNamespaces.STYLES, styleType, jsonValue);
+              }
+            } else if (styleName.includes(oldName)) {
+              const newValue = styleName.replace(oldName, newName);
+              const jsonValue = JSON.stringify(newValue);
+              node.setSharedPluginData(SharedPluginDataNamespaces.STYLES, styleType, jsonValue);


Same for styles

…into 2120-allow-for-bulk-remapping-with-variables-styles

six7

can you also make sure to revert the SharedPluginDataNamespaces.STYLES and SharedPluginDataNamespaces.VARIABLES code? we dont need that now that you've reverted it

Also, a test case would be great.

package.json

six7 · 2023-12-08T07:52:29Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

+                  const paint = figma.util.solidPaint(resolvedValue?.value as string);
+                  const fillsCopy = clone(node.fills);
+                  fillsCopy[0] = figma.variables.setBoundVariableForPaint(paint, 'color', variable);
+                  const newLocalVariable = getLocalVariableByName(resolvedValue?.name as string);


This will only work for local variables. In most cases, users don't have variables in the same file. Can you refactor this so it's fetching the variable by key (i think https://www.figma.com/plugin-docs/api/properties/figma-variables-importvariablebykeyasync/ should do the trick, we're using it elsewhere)

…into 2120-allow-for-bulk-remapping-with-variables-styles

six7 · 2023-12-10T18:28:36Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

-  return localVariables.find((variable) => variable.resolvedType === 'COLOR' && variable.name.split('/').join('.') === name);
-};
+const getStyleByName = (name: string) => (
+  figma.getLocalPaintStyles().find((style) => style.name === name)


same for styles, users will not have those styles local, they will be remote. we need to fetch those from the remote.

six7 · 2023-12-10T18:28:52Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

@@ -50,7 +50,7 @@ export const bulkRemapTokens: AsyncMessageChannelHandlers[AsyncMessageTypes.BULK
          }
        });

-        if (Object.keys(tokens).length > 0) {
+        if (Object.keys(tokens).length === 0) {


is this on purpose? this would mean this would never run

six7 · 2023-12-10T18:29:59Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

@@ -63,13 +63,20 @@ export const bulkRemapTokens: AsyncMessageChannelHandlers[AsyncMessageTypes.BULK
                  const paint = figma.util.solidPaint(resolvedValue?.value as string);
                  const fillsCopy = clone(node.fills);
                  fillsCopy[0] = figma.variables.setBoundVariableForPaint(paint, 'color', variable);
-                  const newLocalVariable = getLocalVariableByName(resolvedValue?.name as string);
-                  fillsCopy[0].boundVariables.color.id = newLocalVariable?.id;
+                  const newVariable = await figma.variables.importVariableByKeyAsync(resolvedValue?.name as string);


have you tried if it's faster if you store these in a cache? i assume if a user has 100 variables applied and remaps them, with this current approach we call importVariableByKeyAsync 100 times. if we had it in store we would call it 1 time.

six7 · 2023-12-10T18:30:26Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

+          if (getAppliedStylesFromNode(node).length > 0) {
+            const { name: styleName } = getAppliedStylesFromNode(node)[0];
+            if (node.type !== 'DOCUMENT' && node.type !== 'PAGE' && 'fills' in node) {
+              const newStyle = getStyleByName(styleName.replace(oldName, newName).split('.').join('/'));


as mentioned above, this currently will just work for local styles. users will likely almost never have these locally.

…named

six7 · 2024-01-11T18:09:08Z

src/constants/SharedPluginDataNamespaces.ts

+  VARIABLES = 'variables',
+  STYLES = 'styles',


not needed anymore

six7 · 2024-01-11T18:12:41Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

+
+          if (Object.keys(tokens).length === 0) {
+            if (getAppliedVariablesFromNode(node).length > 0) {
+              const { name: variableName } = getAppliedVariablesFromNode(node)[0];


does this mean we only pick the first variable from all applied properties? what if there's a color, a width, a height, a spacing variable attached?

six7 · 2024-01-11T18:14:12Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

+              const effectStyles = figma.getLocalEffectStyles();
+              const paintStyles = figma.getLocalPaintStyles();
+              const textStyles = figma.getLocalTextStyles();
+              const allStyles = [...effectStyles, ...paintStyles, ...textStyles];


this is something that happens on every token, and on every node now. move this code outside of the main function to just perform it once and then remember those

six7 · 2024-01-11T18:15:07Z

src/plugin/asyncMessageHandlers/bulkRemapTokens.ts

+              const textStyles = figma.getLocalTextStyles();
+              const allStyles = [...effectStyles, ...paintStyles, ...textStyles];
+
+              const themeInfo = await AsyncMessageChannel.PluginInstance.message({


again, this happens on every node and on every token, this has got to be costly for performance. move it outside the main function and just trigger it once

feat: allow for bulk remap with variable and styles

c73f450

add changeset

48078ce

robinhoodie0823 requested review from SorsOps and six7 December 4, 2023 06:52

SorsOps reviewed Dec 4, 2023

View reviewed changes

.changeset/unlucky-forks-mate.md Outdated Show resolved Hide resolved

robinhoodie0823 self-assigned this Dec 4, 2023

robinhoodie0823 linked an issue Dec 4, 2023 that may be closed by this pull request

Allow For Bulk Remapping With Variables & Styles #2120

Closed

robinhoodie0823 requested a review from SorsOps December 4, 2023 07:46

six7 requested changes Dec 5, 2023

View reviewed changes

robinhoodie0823 added 3 commits December 7, 2023 01:47

Merge branch 'main' of https://github.com/tokens-studio/figma-plugin …

c7c0e80

…into 2120-allow-for-bulk-remapping-with-variables-styles

refactor bulk remapping using bound variables

7ff3ace

add test case

c64388e

robinhoodie0823 requested review from six7 December 7, 2023 19:15

six7 requested changes Dec 8, 2023

View reviewed changes

robinhoodie0823 added 4 commits December 8, 2023 09:15

add bulk remap to styles

19e22d3

revert to not use shared plugin data for variables and styles

84f7273

Merge branch 'main' of https://github.com/tokens-studio/figma-plugin …

8a6664d

…into 2120-allow-for-bulk-remapping-with-variables-styles

revert to net use new tokens

90af891

six7 requested changes Dec 10, 2023

View reviewed changes

robinhoodie0823 added 6 commits December 27, 2023 08:17

refactor: refactor to update token when its referenced other token re…

3f0f62c

…named

Merge branch 'main' of https://github.com/tokens-studio/figma-plugin

6571525

logs to figure out the issue

8ade071

get style key

46010a2

logs to figure out the issue

eb2449e

update the logic of bulk remapping styles

5d5a8c7

robinhoodie0823 added 2 commits January 11, 2024 04:30

Merge branch 'main' of https://github.com/tokens-studio/figma-plugin

87fbfb1

resolve merge conflict and update test case

007adce

robinhoodie0823 requested a review from six7 January 11, 2024 08:37

six7 requested changes Jan 11, 2024

View reviewed changes

improve performance

370aa5c

robinhoodie0823 requested a review from six7 January 12, 2024 08:48

robinhoodie0823 added 4 commits January 12, 2024 04:50

update to handle mutiple variables and styles

5ef058a

resolve eslint error

8473c71

refactor: change the way to get theme info

5130776

refactor typo

c376148

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow for bulk remap with variable and styles #2405

allow for bulk remap with variable and styles #2405

robinhoodie0823 commented Dec 4, 2023

changeset-bot bot commented Dec 4, 2023 •

edited

github-actions bot commented Dec 4, 2023

github-actions bot commented Dec 4, 2023 •

edited

github-actions bot commented Dec 4, 2023 •

edited

six7 Dec 5, 2023

six7 Dec 5, 2023

six7 left a comment

six7 Dec 8, 2023

six7 Dec 10, 2023

six7 Dec 10, 2023

six7 Dec 10, 2023

six7 Dec 10, 2023

six7 Jan 11, 2024

six7 Jan 11, 2024

six7 Jan 11, 2024

six7 Jan 11, 2024

allow for bulk remap with variable and styles #2405

Are you sure you want to change the base?

allow for bulk remap with variable and styles #2405

Conversation

robinhoodie0823 commented Dec 4, 2023

changeset-bot bot commented Dec 4, 2023 • edited

🦋 Changeset detected

github-actions bot commented Dec 4, 2023

PR Analysis

PR Feedback

How to use

github-actions bot commented Dec 4, 2023 • edited

Test coverage results 🧪

github-actions bot commented Dec 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

six7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Dec 4, 2023 •

edited

github-actions bot commented Dec 4, 2023 •

edited

github-actions bot commented Dec 4, 2023 •

edited