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

feat: ignore first part of token name for variables #1992

Merged

Conversation

mihkeleidast
Copy link
Contributor

Applies the same behavior to variables as could be previously done to styles.

Let me know if this should be done differently in some way (e.g. a separate settings value?).

@six7
Copy link
Collaborator

six7 commented Jul 26, 2023

Sorry for the delay on this, we're currently focused on performance. I'll make sure to focus on this for a release after the performance focus is over 🙏

@railstride
Copy link

Hey @six7, were you able to make any progress on this? Very keen to "just" check the option rather than rewriting the core of a style dictionary :D

@six7
Copy link
Collaborator

six7 commented Sep 1, 2023

Hey @six7, were you able to make any progress on this? Very keen to "just" check the option rather than rewriting the core of a style dictionary :D

hey, no, we're still doing the perf release

@mihkeleidast
Copy link
Contributor Author

@six7 Any updates, when should we expect progress on this? It's getting a bit annoying to have to rely on a feature branch to sync variables properly...

@six7
Copy link
Collaborator

six7 commented Nov 22, 2023

@six7 Any updates, when should we expect progress on this? It's getting a bit annoying to have to rely on a feature branch to sync variables properly...

Ah, sorry for updating. Feel free to rebase the PR on main and recreate. Just add tests to make sure the functionality works as expected 🙏

@mihkeleidast mihkeleidast force-pushed the ignore-first-part-of-token-name-for-vars branch from 6203223 to 8d582ee Compare November 22, 2023 11:28
Copy link

changeset-bot bot commented Nov 22, 2023

🦋 Changeset detected

Latest commit: 35ae337

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

@mihkeleidast
Copy link
Contributor Author

@six7 Cheers, done! I have one open question regarding whether this feature should use the existing setting, or a new setting?

Also, not sure if I have to do anything to get the failing checks to succeed, does not seem like an issue I have created?

@LukeFinch
Copy link
Contributor

@six7 Cheers, done! I have one open question regarding whether this feature should use the existing setting, or a new setting?

Also, not sure if I have to do anything to get the failing checks to succeed, does not seem like an issue I have created?

HI @mihkeleidast I'll check the tests for you. As for whether it should be a new setting. I think for now - let's keep it as a separate setting. It's easier to merge them in the future if we want to. There may also be a discovery benefit from us having it called out individually.

@mihkeleidast
Copy link
Contributor Author

Refactored it to a separate settings value.

Copy link
Collaborator

@six7 six7 left a comment

Choose a reason for hiding this comment

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

Thanks for all this! Code looks fine, but: As you have it right now, this setting will not be remembered each time the user restarts the plugin.

Look into updateUISettings to see how we do this, there you'll notice we pass through the ignoreFirstPartForStyles. As a user I'd expect to set this once and forget about it, not have to recheck that everytime I need it

@mihkeleidast mihkeleidast requested a review from six7 November 27, 2023 07:40
Copy link
Collaborator

@six7 six7 left a comment

Choose a reason for hiding this comment

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

Nice, thank you! I'll merge this in - we'll combine that with another release this week

@six7 six7 merged commit 28cd5d7 into tokens-studio:main Nov 27, 2023
5 of 7 checks passed
LukeFinch added a commit that referenced this pull request Dec 5, 2023
@railstride
Copy link

been following this issue and am a bit confused about the current state 😄
so the branch got merged, then reverted due to a bug of this branch and again merged with the bug fix (1.38.6)
… so the functionality is there, right? Now the UI is still missing the option to actually set this feature, or am I missing something?

@esthercheran
Copy link
Collaborator

Hey @railstride we reverted this as it wasn't working as expected some additional work is needed. Will keep you updated.

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.

5 participants