When switching themes switch the figma native theme mode selector if possible #2336

robinhoodie0823 · 2023-10-25T17:38:50Z

20231025_133845.mp4

Closes #2187

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

changeset-bot · 2023-10-25T17:38:53Z

🦋 Changeset detected

Latest commit: 21f2ae5

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-10-25T17:40:42Z

PR Analysis

🎯 Main theme: The main theme of this PR is to switch the Figma native theme mode selector when switching themes in the plugin.
📝 PR summary: This PR introduces changes to the AsyncMessageChannel and setActiveTheme files. It adds functionality to switch the Figma native theme mode selector when switching themes in the plugin. It also updates the state when a new active theme is set.
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
⏱️ Estimated effort to review [1-5]: 3, The PR introduces a new feature and modifies the state management, which requires a good understanding of the existing codebase and the new feature being introduced. However, the changes are not extensive and should be manageable to review.
🔒 Security concerns: No

PR Feedback

💡 General suggestions: The code changes seem to be well implemented, but it would be beneficial to add error handling for the new code blocks. Also, it would be good to consider the performance implications of the forEach loop on figma.currentPage.children.
🤖 Code feedback:
- relevant file: src/AsyncMessageChannel.ts
  suggestion: Consider adding error handling for the new code block. If there's an error while setting the explicit variable mode for a collection, it should be caught and handled appropriately. [important]
  relevant line: figma.currentPage.children.forEach((frame) => {
- relevant file: src/AsyncMessageChannel.ts
  suggestion: The forEach loop on figma.currentPage.children could potentially be expensive if there are many children. Consider if there's a more efficient way to achieve the same result. [medium]
  relevant line: figma.currentPage.children.forEach((frame) => {

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

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

SorsOps · 2023-10-26T10:21:49Z

src/AsyncMessageChannel.ts

@@ -122,6 +122,16 @@ export class AsyncMessageChannel {
      this.attachMessageListener((msg: IncomingMessageEvent<AsyncMessageResults & { type: Message['type'] }>['data']['pluginMessage']) => {
        if (msg.id === messageId) {
          if ('message' in msg) {
+            if (msg.message.type === 'async/get-theme-info') {
+              if ('activeTheme' in msg.message && 'themes' in msg.message) {
+                const { activeTheme } = msg.message;


Suggested change

const { activeTheme } = msg.message;

const { activeTheme, themes } = msg.message;

SorsOps · 2023-10-26T10:21:58Z

src/AsyncMessageChannel.ts

+            if (msg.message.type === 'async/get-theme-info') {
+              if ('activeTheme' in msg.message && 'themes' in msg.message) {
+                const { activeTheme } = msg.message;
+                const { themes } = msg.message;


Suggested change

const { themes } = msg.message;

const themeToFind = Object.values(activeTheme)[0];

SorsOps · 2023-10-26T10:23:28Z

src/AsyncMessageChannel.ts

+              if ('activeTheme' in msg.message && 'themes' in msg.message) {
+                const { activeTheme } = msg.message;
+                const { themes } = msg.message;
+                const activeMode = themes.filter((theme) => theme.id === Object.values(activeTheme)[0])[0];


Suggested change

const activeMode = themes.filter((theme) => theme.id === Object.values(activeTheme)[0])[0];

const activeMode = themes.find((theme) => theme.id === themeToFind);

SorsOps · 2023-10-26T10:25:12Z

Can you add a changeset so we can inform the users about this change?

Check to see if the fixes for the lookup fix the OOM error, otherwise we'll have to double check why that's occurring

six7 · 2023-11-10T16:35:43Z

Tests are failing here

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

github-actions · 2024-01-10T01:01:04Z

Commit SHA:4c507f93be36defe7933184695662c0f75d72629

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

Status	File	% Stmts	% Branch	% Funcs	% Lines
🔴	total	70.73 (0.01)	61.08 (-0.02)	68.84 (-0.02)	70.98 (0.01)
🔴	src/AsyncMessageChannel.ts	94.91 (-3.09)	82.6 (-14.27)	92.85 (-7.15)	94.64 (-3.27)

github-actions · 2024-01-10T01:01:06Z

Commit SHA:4c507f93be36defe7933184695662c0f75d72629
Current PR reduces the test coverage percentage by 100 for some tests

SorsOps · 2024-01-10T08:33:36Z

.changeset/shy-cycles-develop.md

@@ -0,0 +1,5 @@
+---


This looks redundant?

SorsOps · 2024-01-10T08:35:45Z

src/AsyncMessageChannel.ts

+                const activeMode = themes.find((theme) => theme.id === themeToFind);
+                if (figma.currentPage.children && figma.currentPage.children.length > 0) {
+                  figma.currentPage.children.forEach((frame) => {
+                    frame.setExplicitVariableModeForCollection(activeMode?.$figmaCollectionId ?? '', activeMode?.$figmaModeId ?? '');


Shouldn't we be using an if around this and checking that the string id's exist? What happens when we these are invalid or null?

yeah it could be that the information is outdated and then this will error. they'll likely not be invalid or null but it could be that it doesnt exist anymore causing an error.

let's check for if a) those are set and b) wrap so that we dont crash

six7 · 2024-01-10T12:34:40Z

src/AsyncMessageChannel.ts

@@ -122,6 +122,18 @@ export class AsyncMessageChannel {
      this.attachMessageListener((msg: IncomingMessageEvent<AsyncMessageResults & { type: Message['type'] }>['data']['pluginMessage']) => {
        if (msg.id === messageId) {
          if ('message' in msg) {
+            if (msg.message.type === 'async/get-theme-info') {


i dont think this belongs here. this file doesnt have any of the other message handlers, this needs to go elsewhere where we define those.

figma-plugin/src/app/components/Initiator.tsx

Line 31 in 1424c9f

switch (pluginMessage.type) {

this feels like the right place

six7 · 2024-01-10T12:35:53Z

src/AsyncMessageChannel.ts

+                const activeMode = themes.find((theme) => theme.id === themeToFind);
+                if (figma.currentPage.children && figma.currentPage.children.length > 0) {
+                  figma.currentPage.children.forEach((frame) => {
+                    frame.setExplicitVariableModeForCollection(activeMode?.$figmaCollectionId ?? '', activeMode?.$figmaModeId ?? '');


yeah it could be that the information is outdated and then this will error. they'll likely not be invalid or null but it could be that it doesnt exist anymore causing an error.

let's check for if a) those are set and b) wrap so that we dont crash

six7 · 2024-01-10T12:38:44Z

src/AsyncMessageChannel.ts

+                const themeToFind = Object.values(activeTheme)[0];
+                const activeMode = themes.find((theme) => theme.id === themeToFind);
+                if (figma.currentPage.children && figma.currentPage.children.length > 0) {
+                  figma.currentPage.children.forEach((frame) => {


this ignores the issue requirement completely and sets an active variable mode for every frame on the current page. this is not what we need. we need to apply way more logic here than that.

#2187

the issue states that it should have different behavior depending on the apply mode

six7 · 2024-01-10T12:39:16Z

src/AsyncMessageChannel.ts

+            if (msg.message.type === 'async/get-theme-info') {
+              if ('activeTheme' in msg.message && 'themes' in msg.message) {
+                const { activeTheme, themes } = msg.message;
+                const themeToFind = Object.values(activeTheme)[0];


we do not just want to switch the first theme, but all theme groups that are active

robinhoodie0823 added 5 commits October 16, 2023 08:16

add console logs to figure out issues

11ec40d

clean console log

76fb981

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

4fdd735

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

apply style to figma check

a45930e

switching active theme in plugin

99d2004

robinhoodie0823 self-assigned this Oct 25, 2023

robinhoodie0823 requested review from SorsOps and six7 October 25, 2023 17:39

SorsOps requested changes Oct 26, 2023

View reviewed changes

resolve comments

e8026a1

robinhoodie0823 requested a review from SorsOps November 7, 2023 13:17

feat: add changeset

31099b0

robinhoodie0823 added 5 commits December 5, 2023 04:08

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

64a4de2

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

update: update the logic

253eca3

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

c1b0204

…into 2187-when-switching-themes-switch-the-figma-native-theme-mode-selector-if-possible

update to pass test case

3fbd4d7

feat: add changest

21f2ae5

SorsOps reviewed Jan 10, 2024

View reviewed changes

six7 requested changes Jan 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When switching themes switch the figma native theme mode selector if possible #2336

When switching themes switch the figma native theme mode selector if possible #2336

robinhoodie0823 commented Oct 25, 2023 •

edited by SorsOps

Loading

changeset-bot bot commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023

SorsOps Oct 26, 2023

SorsOps Oct 26, 2023

SorsOps Oct 26, 2023

SorsOps commented Oct 26, 2023

six7 commented Nov 10, 2023

github-actions bot commented Jan 10, 2024 •

edited

Loading

github-actions bot commented Jan 10, 2024 •

edited

Loading

SorsOps Jan 10, 2024

SorsOps Jan 10, 2024

six7 Jan 10, 2024

six7 Jan 10, 2024

six7 Jan 10, 2024

six7 Jan 10, 2024

six7 Jan 10, 2024

	const { activeTheme } = msg.message;
	const { activeTheme, themes } = msg.message;

	const { themes } = msg.message;
	const themeToFind = Object.values(activeTheme)[0];

	const activeMode = themes.filter((theme) => theme.id === Object.values(activeTheme)[0])[0];
	const activeMode = themes.find((theme) => theme.id === themeToFind);

When switching themes switch the figma native theme mode selector if possible #2336

Are you sure you want to change the base?

When switching themes switch the figma native theme mode selector if possible #2336

Conversation

robinhoodie0823 commented Oct 25, 2023 • edited by SorsOps Loading

changeset-bot bot commented Oct 25, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 25, 2023

PR Analysis

PR Feedback

How to use

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SorsOps commented Oct 26, 2023

six7 commented Nov 10, 2023

github-actions bot commented Jan 10, 2024 • edited Loading

Test coverage results 🧪

github-actions bot commented Jan 10, 2024 • edited Loading

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

robinhoodie0823 commented Oct 25, 2023 •

edited by SorsOps

Loading

changeset-bot bot commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Jan 10, 2024 •

edited

Loading

github-actions bot commented Jan 10, 2024 •

edited

Loading