-
Notifications
You must be signed in to change notification settings - Fork 569
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
Having a default property in a namespace #119
Comments
Would it be confusing to support this as well?
I know people used to complain about having to add the object and value properties. With our added support for comments (and maybe additional properties in the future), I'm wondering if we want to establish / formalize the object at the end. |
Thinking more on this, I feel weird making "base" an automatic sub-key. @elliotdickison had some good thoughts on base -> components and base -> theme -> components as a dependency chain of design tokens. He also mentioned @bradfrost post on theming with design tokens as similar thinking in this regard. We should make sure our solution is thoughtful here. |
@dbanksdesign, @elliotdickison, @didoo thoughts? |
I guess I'm more willing to have "value" be optional. So using Danny's original example could be rewritten as:
This would directly conflict with Danny's suggestion. Would like to get feedback from folks on what they think would be a more useful solution. |
Having "value" as optional is a lot more of a technical constraint. We need some way of knowing what is a token and the value of a token versus extra data/structure to the JSON object. Without it, we would not know what is a token. |
Somehow I understand why people would like to be able to
but personally I don't think is something we should try to fix in exchange for more complexity in the codebase and less clarity in the properties declarations. As soon as you have a "secondary" font color, from the semantic perspective you have to come up with a name for the other available option, even if is the default one. I can imagine that it may look easier to just write |
Should we close this as "won't fix"? |
@dbanksdesign suggested I throw a use case on the pile. In the Design Systems slack I posed a question based on what I tried and couldn't accomplish: having tokens both for a color (
Danny confirmed this couldn't be done. (I'd missed this issue when searching for answers, so thanks for pointing me here. :) ) I can work around it with a naming convention e.g. So this is not a show-stopper for me (not in the least), but it does seem to be a more valid use case than what you'd mentioned previously. 🤷♂️ Thanks. |
What would be the cons or disadvantages for allowing the following to work? {
"color": {
"font": {
"value": "#111" ,
"secondary": { "value": "#333" },
}
}
} I suppose if this wasn't supported an alternative would be for users to automatically link |
I'd like to retract my previous statements about making value optional. We are putting lots of other things besides values in these files now, and having a way to distinguish a token from any other random data stored in these files is important. |
We've decided to put our optional modifiers at the end and being able to have children when there's a value would be very helpful. I suppose we could add a pseudo-child called 'base' or 'normal', but really that just adds clutter. Consider the ramifications for nested cases such as ours. We have a button which can be primary or secondary. Each of these can be regular or inverse (against a dark background) It can have states such as hover and active.
Should we really have to define a base version of each of these modifiers? What if we add a pressed of disabled state? I Personally find the above to be much more readable than:
Also, I think each optional attribute should use a different adjective for "normal" or "base". ("notinverse"? "verse"?):
I went through the exercise of redoing the button definition using only tokens named with the above "complete" requirement. I found the tokens to be nigh unreadable. It was nearly impossible to find any of the values I needed as I was combing through the less. I would really like to see style dictionary support children for nodes with a value. I'm currently exploring the use of a preprocessor to flatten out the hierarchy down to the values, which should allow me to use the json I have in mind. That's my two cents. |
One approach to this that I've found to work is using a special character to define the base style. The only place this breaks down is aliasing.
outputs
In order to alias the token example above, you need to include the Alternatively, you could flip this on it's head and create a way to define sub-tokens in a similar way.
|
IMHO, the {
"color": {
"global": {
"background": {
"value": "#FFFFFF",
"variants": {
"inverse": {
"value": "#000000"
}
}
}
}
}
} |
@ryanfitzer that sounds like a great idea. The only reason I was straying away from any particular word was to not add limitations to the name of a token. That being said, |
I like @ryanfitzer idea with |
I have this issue, I need to replace a tailwind config with style generated output, but the original config was made with |
@dbanksdesign
|
@uptonking you can take a look at the discussion on #575 where we are talking about that. I 100% know how annoying and tedious it is writing |
@dbanksdesign may I suggest to rename this issue from "Having a default property in a namespace" to "Having tokens as siblings of a I struggled finding this issue when looking for informations about this: https://twitter.com/nhoizey/status/1397597969457852416 |
A few people have tried to create a structure where there is a "default" or "base" property on a given namespace, without having to name the property "default" or "base". For example, rather than doing this:
Doing this:
This is so you could reference
color.font
as well ascolor.font.secondary
or$color-font
in scss. This is not possible today and would require some code changes to how style dictionary traverses the properties object.Let's use this issue to discuss the pros/cons of this approach.
The text was updated successfully, but these errors were encountered: