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 empty modify on JSON save or edit #2816

Merged
merged 9 commits into from
Jun 16, 2024
Merged

Conversation

macintoshhelper
Copy link
Contributor

@macintoshhelper macintoshhelper commented Jun 5, 2024

Why does this PR exist?

Closes #2805

What does this pull request do?

  • Adds a validateStudioTokensExtensions util function to strip empty studio.tokens extensions such as modify.
  • Fix JSON not updating on first paste/edit by removing a useEffect dependency on getStringTokens.
Before After
image image
image image

Testing this change

To reproduce

  1. Go to JSON view
  2. Paste this JSON:
{
 "blue": {
    "value": "#ff0000",
    "type": "color",
    "$extensions": {
      "studio.tokens": {}
    }
  },
  "red": {
    "value": "#ff0000",
    "type": "color",
    "$extensions": {
      "studio.tokens": {
        "modify": {}
      }
    }
  },
 "valid": {
    "value": "#ff0000",
    "type": "color",
    "$extensions": {
      "studio.tokens": {
        "modify": {
          "type": "darken",
          "space": "srgb",
          "value": "0.5"
        }
      }
    }
  }
}

For existing tokens with empty modify:

  1. Right click and click Edit Token, then save.
  2. The empty modify should be stripped.

Additional Notes (if any)

A migration step would be nice to automatically strip empty extension objects when the plugin is opened/upgraded, but this would probably be better left for a future migration system with strong testing/error catching.

Copy link

changeset-bot bot commented Jun 5, 2024

⚠️ No Changeset found

Latest commit: a880422

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -179,7 +179,7 @@ function Tokens({ isActive }: { isActive: boolean }) {
// because of specific logic requirements
setError(null);
dispatch.tokenState.setStringTokens(getStringTokens());
}, [tokens, activeTokenSet, tokenFormat, tokenType, dispatch.tokenState, getStringTokens]);
}, [tokens, activeTokenSet, tokenFormat, tokenType, dispatch.tokenState]); // getStringTokens removed to fix bug around first paste/edit (useEffect was being triggered)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my testing, on the first edit of stringTokens, getStringTokens was changing, triggering this useEffect to run with a race condition, wiping the initial change/paste.

This should fix it, but additional testing would be nice :)

if (typeof payload?.$extensions?.['studio.tokens'] !== 'undefined') {
studioTokensExtension = Object.keys(payload?.$extensions?.['studio.tokens']).reduce((accExt, k) => {
const extension = (payload?.$extensions?.['studio.tokens'] || {})[k];
if (extension && Object.values(extension).length > 0) {
Copy link
Contributor Author

@macintoshhelper macintoshhelper Jun 5, 2024

Choose a reason for hiding this comment

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

This should look through every value of the studio.tokens extension, and if an object with no values is found (i.e. modify: {}), it should filter it out (avoid adding it to the accumulator object).

@macintoshhelper macintoshhelper marked this pull request as ready for review June 5, 2024 13:47
@cuserox
Copy link
Contributor

cuserox commented Jun 6, 2024

✅ Tested by copy pasting the JSON, and it works!

🤔 This might not be in scope for the fix, but now that there's the validateTokensStudioExtension helper, it could be applied to other cases where these empty modify object appears. I tried replicating the issue being faced by this user(for example, the border-radius video)

With your branch locally:

  • Go into renaming a token, which opens the modal and rename the token
  • See the modal that asks to remap the tokens that use it
  • Save it

😫 This also adds an empty modify object

My hunch is that the remapping action is adding it somewhere, maybe? REMAP_TOKENS

Screenshot 2024-06-06 at 16 48 46
renameTokenModifyObject.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great candidate to write some unit tests 🤓

@six7 six7 linked an issue Jun 10, 2024 that may be closed by this pull request
@six7 six7 merged commit 9173fb8 into release-139 Jun 16, 2024
7 of 8 checks passed
@six7 six7 deleted the fix/2805-empty-modify branch June 16, 2024 11:12
@six7 six7 added the 2.0-rc8 label Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.0: Empty modify being saved to JSON
3 participants