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(json/original): Add json/original format #737

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

theKosh
Copy link

@theKosh theKosh commented Nov 21, 2021

Issue #727

Description of changes:

  • The json/original format allows you to leave only those keys that were specified in the original object without additional metadata. More info [Question] - Is there a way to not include default metadata when formatting in a JSON format? #727.

  • Added dictionary filtered function:
    In:

    • obj - The object to filter. You will most likely pass dictionary.tokens.color to it.
    • exclude - The keys that need to be excluded from the filtered object
    • target - The keys that must be included in the result object.

    Out: the object which contains only given keys.

Tested with the Figma Tokens plugin.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@chazzmoney
Copy link
Collaborator

Hi @theKosh , thanks for your contribution!

This is a good idea, but needs some changes.

I'll add all of the comments below as a review, but I wanted to try to capture all the thinking in one spot:

  • Adding a function to filter token metadata is a great idea that may have utility for a wide variety of SD users. Can we get another function that does the same exact thing but uses an "include" list instead of an "exclude" list?
  • Adding a format for this is.... confusing. Formats are intended to be for platform style usage elsewhere. I'm unclear how this would be widely beneficial - or rather, how this would be useful to many/most StyleDictionary users
  • There are a couple things that need to be cleaned, like changing the version number / package.lock

Thanks again for your contribution! Looking forward to adding in this functionality.

Copy link
Collaborator

@chazzmoney chazzmoney left a comment

Choose a reason for hiding this comment

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

Nice work. A few changes to get it all in line for merge. Thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
__tests__/formats/__snapshots__/all.test.js.snap Outdated Show resolved Hide resolved
__tests__/formats/jsonOriginal.test.js Outdated Show resolved Hide resolved
examples/advanced/assets-base64-embed/package.json Outdated Show resolved Hide resolved
examples/advanced/auto-rebuild-watcher/package.json Outdated Show resolved Hide resolved
lib/common/formats.js Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/common/formatHelpers/filterDictionary.js Outdated Show resolved Hide resolved
__tests__/common/formatHelpers/filterDictionary.test.js Outdated Show resolved Hide resolved
@theKosh
Copy link
Author

theKosh commented Jan 24, 2022

Hi @chazzmoney.

Can we get another function that does the same exact thing but uses an "include" list instead of an "exclude" list?

Yes, okay, I'll do it.

Adding a format for this is.... confusing. Formats are intended to be for platform style usage elsewhere. I'm unclear how this would be widely beneficial - or rather, how this would be useful to many/most StyleDictionary users

For example:

  1. When you need to change tokens via transform, but you need to leave the json format as the original one, since you don't need extra metadata. In my case, for the web, I applied the rule "transforms['color/css2']".
  2. When tokens are generated for many platforms, but no changes are required for one of them, at the same time, it is more convenient for developers to have everything in one place in the build folder along with the processed assets. In this case, we can do it through the fs.copyfilesync function, but doing it through the format is more flexible, since it allows you to use transforms, manage assets, etc.

There is a discussion of the problem in this topic.

There are a couple things that need to be cleaned, like changing the version number / package.lock

I'll fix it.

Thanks for the detailed code review. I'll try to do everything in my spare time.

@theKosh
Copy link
Author

theKosh commented May 15, 2022

Hi, @chazzmoney.

I fixed all the errors and added the excludeTokensMetadata method as you asked.

Sorry it took so long — I got tied up at work.

@theKosh theKosh requested a review from chazzmoney August 1, 2022 08:18
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.

3 participants