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(dictionary) Added support of property private for design tokens interface DesignToken #704

Closed
wants to merge 3 commits into from

Conversation

silversonicaxel
Copy link
Contributor

@silversonicaxel silversonicaxel commented Sep 28, 2021

Description of changes:
This update of the interface DesignToken allows to set a design token to private: true in order to avoid to be outputted in the distribution folder after the build process.

For example, having an initial source of design tokens like the following:

{
  'black': {
    'value': '#000000',
    'private': true
  },
  'aqua': {
    'value': '#00FFFF'
  },
  'white': {
    'value': '#FFFFFF',
    'private': false
  }
}

a potential scss output would be something like this:

$aqua: '#00FFFF';
$white: '#FFFFFF';

or a potential json output would be something like this:

{
  'aqua': '#00FFFF',
  'white': '#FFFFFF'
}

The reason why I added this feature is the need of having hidden design tokens, relevant to be in the source code but not important for the final output distribution.

Currently there is already a way to filter out design tokens with this approach https://amzn.github.io/style-dictionary/#/api?id=registerfilter and https://amzn.github.io/style-dictionary/#/formats?id=filtering-tokens filter based.

What I implemented, it looks to me like a complementary feature, but this private property way avoids the creation of filters to apply to the creation process, giving a filtering result in a faster way and with less code.

Thanks for the attention.

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

chazzmoney commented Oct 14, 2021

Hi @silversonicaxel !

Thank you for your submission. I like the direction you are going here...

Generally we try to provide as much flexibility as possible and as few prescriptive techniques as possible. If someone has created a "private" attribute in their own project, updating SD may break their build. Additionally, someone else may have created a CTI structure to specifically remove private tokens instead of an attribute, and this would require them to switch.

Instead, what if we added a built-in filter that utilized the property? Then exiting builds would not break.

To further support your desire for smaller configuration, what if we also added top-level configuration that cascaded down to platforms? (the same way that platform level config cascades down to files). This would mean you simply type in the name of the pre-existing filter as your top-level filter, which would then cascade down.

Would this be a good solution?

@silversonicaxel
Copy link
Contributor Author

Hello @chazzmoney, I understand that my approach could potentially break some builds that were using accidentally a custom private attribute. That is indeed the risk for any new possible attribute that SD might need in the future.

I guess another approach for filtering could be yes your proposal, a built-in filter is fine by me, if it can help in achieving faster a filter, instead of writing it myself (although it is not a huge amount of code, though).
Agreed!

@chazzmoney
Copy link
Collaborator

@silversonicaxel Just want to reconnect on this.

Did you want to update the PR with a built-in filter and a cascading property? Or do you have questions on how to go about doing any of that?

Looking forward to being able to support private tokens!

@silversonicaxel
Copy link
Contributor Author

@chazzmoney sure let's continue the conversation, glad to help. I'm wondering how would you approach the built-in filter, since there are chances to work on built-in filter on some formats/templates, but in this case the need would be for every type of design tokens. Maybe I miss some pictures of the entire StyleDictionary library.

@caseybaggz
Copy link

Definitely need this feature. Any ideas on when it will be merged and released?

@silversonicaxel
Copy link
Contributor Author

@caseybaggz could you eventually also review the PR?

I believe this PR could be merged, but if this current work is not enough, I am available to proceed the conversation that @chazzmoney was suggesting. But I believe that conversation got stuck.

I've no visibility on roadmap @caseybaggz

@didoo
Copy link
Contributor

didoo commented Jan 26, 2022

Suggestion: when working on this take in account what the Design Tokens Community Group may want to define in their specs about this kind of entities (eg. when a token is used as alias, and one don't want to generate it in output).

I would not be surprised if this has been already discussed in one of their forums (or you can reach out to them, I think a @dbanksdesign has something to do with it :) ) and see if they are considering the private name for this scope. In which case, if it becomes a standard of a token, you may decide to have a different approach and not consider it anymore a custom property (of course that would be a breaking change for other users that may be using private with another meaning/intention).

@silversonicaxel
Copy link
Contributor Author

Thanks @didoo for the great insight, I was indeed not considering the Design Tokens Community Group honestly, probably because in my mind the idea of setting a design token private, just for internal purposes, doesn't seem so disruptive or in need of a "blessing" from above.
But it is nice that someone brought it up, so thanks for that.

I could reach them out asking their opinion about this PR, this approach. Indeed it would be nice to hear also something from the creators of this repository, like indeed @dbanksdesign or who else, if it might be good or not.

And, being breaking change, well, that's life of packages :)

@chazzmoney
Copy link
Collaborator

Hey everyone, thanks for the great comments, just wanted to add an update from our end.

This is definitely a priority for us as we think it will be useful. Currently we have focused meetings on Tuesday and Thursday and at the end of of our meeting yesterday we decided that Thursday’s primary focus would be on this ticket.

Generally, we want this to be merged but we need an option in the configuration to be added. More specifics tomorrow!

@silversonicaxel
Copy link
Contributor Author

Thanks @chazzmoney for the heads-up !

@chazzmoney
Copy link
Collaborator

chazzmoney commented Jan 28, 2022

Ok, so. This functionality is great but it doesn't fit with the current organization of the architecture.

Instead, we should create a filters.js file at /lib/common/filters.js, (similar in structure to formats.js This file will hold all the built-in filters (which currently does not exist). This file then needs to be loaded into StyleDictionary here.

Then you have to add the removePrivate function to the filters.js file. It should be function that takes a token and returns a boolean if the token should be included (true) or excluded (false). Something like (token) => !token.private.

This would be a non-breaking change. It would enable anyone to remove tokens that contain the "private: true" metadata by adding the "removePrivate" filter to their configuration.

I think this is the fastest way to get the functionality out to people effectively. Thoughts? Are you up for doing this work?

@silversonicaxel
Copy link
Contributor Author

I can take care of it. As soon as I've a new branch / PR I will link it to this. In the meanwhile, I would leave this PR open as reference. As soon as it is ready, I will ping you Style Dictionary people, to check that out!

@silversonicaxel
Copy link
Contributor Author

Here we go -> #770

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.

None yet

5 participants