design-tokens / community-group

This is the official DTCG repository for the design tokens specification.
https://tr.designtokens.org
Other
1.56k stars 63 forks source link

Group & file level properties #72

Closed kevinmpowell closed 2 years ago

kevinmpowell commented 3 years ago

Several issues have raised the need for adding token properties at group and file levels. I'm creating this issue to consolidate discussions about this topic.

For example Types:

{
  "colorTextPrimary": {
    "value": "#000000",
    "type": "Color"  // Tedious
  },
  "colorTextSecondary": {
    "value": "#333333",
    "type": "Color" // Tedious
  },
  ...
}

///  VS  ///
{
  "color": {
    "type": "Color", // Define once for the grouping
    "textPrimary": {
      "value": "#000000",
    },
    "textSecondary": {
      "value": "#333333",
    },
  ...
  }
}

Or descriptions for documentation about an entire group:

{
  "colorText": {
    "type": "Color",
    "description": "Our palette of colors for text only.",
    "primary": {
      "value": "#000000",
      "description": "Use as the default text color",
    },
    "secondary": {
      "value": "#333333",
      "description": "Use for text that is not as important."
    },
  ...
  }
}
c1rrus commented 3 years ago

I'm definitely in favour of allowing both description and type on groups as well as tokens. Furthermore, if/when we add any other properties in future we should always consider if they are applicable to tokens, groups or both.

Note, that with type, I think its meaning when applied to a group should be subtly different to when it's applied to a token:

Example:

{
  "group 1": {
    "type": "color",

    "token A": {
      // This token has the type color because it inherits it from group 1
      "value": "#00ff00"
    },

    "token B": {
      // This token has the type dimension, because its own type property
      // takes precedence the inherited color type
      "type": "dimension",
      "value": "12rem"
    },

    "group 1.1": {
      "token B": {
        // This token also has type color because it inherits it from group 1
        // since that is the nearest group with a type property
        "value": "#ff0000"
      }
    },

    "group 1.2": {
      "type": "duration",

      "token C": {
        // This token has type duration because it inherits it from group 1.2
        // (group 1.2's type takes precedence over its parent group's type)
        "value": "150ms"
      }
  },

  "token X": {
    // This token is not within a group with a type, so it does not inherit a type.
    // It also does not have its own type property. Therefore, tools must fall back
    // to the JSON type of its value, which is "string".
    // This token therefore has type "string" (don't be fooled by its value which
    // *looks* like a color!)
    "value": "#abcdef"
  }
}
firede commented 3 years ago

The way to define the token is very free, and the handwriting experience is good. However, if the tokens are modified by tools, the consistency of the format is difficult to guarantee. After each update, the token may have more unexpected diffs.

gossi commented 3 years ago

Setting rules on groups will mean this:

Groups have an impact on tokens within them - or - reading the inverse tokens become dependent on the group they are in.

Case A: Groups are only there for organizing tokens, then this shouldn't be allowed as it shouldn't matter in which group a token is in.

Case B: Groups become contexts. Means, moving a token from one group to another will change its meaning.

Let's make this an example:

{
  "colors": {
    "type": "color",
    "example-token": {
      "value": "#AAA"
    }
  },

  "dimensions": {
    "type": "dimension",
    "example-token": {
      "value": "#AAA"
    }
  }
}

So, what shall be the algorithm to resolve the value of a token? Possible scenarios for case B:

If groups act as context, then the spec shall deliever a resolving algorithm for how to reach at the value. I can compare it to the heading resolving algorithm for html.

The benefit I see is in manually writing groups of tokens all of one type and only defining it once. More chill authoring tokens for sure.

I consider this spec to describe the storage of tokens only and would avoid situations where reading rules become necessary.

sbaechler commented 3 years ago

Could we name group types childrenType instead of type? This would be more specific. It would also prevent bugs when group nodes are changed to leaf nodes.

I disagree with @gossi here that the token file should just be a dumb interchange format. The format is targeted to humans first, not machine first.

I can see a lot of developers and designers editing the file directly with minimal help from tools. Therefore it should be as human-readable as possible. And #AAA should throw an error when used as a dimension type.

TrevorBurnham commented 3 years ago

I'd suggest that properties specified on a group for tokens within that group to inherit should be specified in a different way than properties that apply to the group itself. Otherwise, there's ambiguity over which properties are inherited and which are not. (For example, is a group-level description inherited?)

My proposal would be something like:

{
  "colorText": {
    "description": "Our palette of colors for text only.",
    "tokenProperties": {
      "type": "color"
    },
    "primary": {
      "value": "#000000",
      "description": "Use as the default text color"
    },
    "secondary": {
      "value": "#333333",
      "description": "Use for text that is not as important."
    }
  }
}

In this example, colorText is a group with only one property (a description), and all tokens within that group inherit a different property (type: "color"). This allows for groups to contain any number of properties of their own, along with any number of inherited properties.

Addendum: Avoiding name collisions
What if a design system wants to use the same name for a token as for a group property? For example, imagine that the team wants to have a text color called "description". An extension to accommodate this would be to allow group properties to be defined within a properties group. For example:
```jsonc { "colorText": { "properties": { "description": "Our palette of colors for text only.", }, "tokenProperties": { "type": "color" }, "description": { "description": "Use for descriptive text, including definition lists and footnotes.", "value": "#3f3f3f" } // other tokens... } } ``` The token/group names `"properties"` and `"tokenProperties"` would be disallowed, but anything else would be valid.
jjcm commented 3 years ago

As a followup to this, are values allowed at a group level? Example as follows:

Let's say I want to define these css vars in a .token.json file:

--background-primary: #333;
--background-primary-hover: #444;
--background-primary-active: #222;

Can I define a value for a group like so?

{
    "background": {
        "primary" {
            "value": "#333",
            "type: "color",
            "hover": {
                value: "#444",
            },
            "active": {
                value: "#222",
            },
        },
    },
}
sbaechler commented 3 years ago

Adding a type property on group level might not be such a good idea after all. It would significantly increase the complexity. While adding the type to each node can be tedious for manual editing, tools wouldn't mind.

Comparing a diff would be easier because one wouldn't have to worry about context.

So instead of trying to solve this problem it might be better to just remove it altogether.

splitinfinities commented 3 years ago

The implicit language for grouping is what is a little confusing to me as I wrote my prototype. From my perspective, groups are the default type for any object, nested or otherwise, until an explicit type is defined.

Let's look at one chunk of the group types sample as it's proposed today.

  "text-size": {
    "type": "dimension",

    "scale": {
      "0": {
        "value": "1rem"
      },
      "1": {
        "value": "1.25rem"
      },
      "2": {
        "value": "1.563rem"
      },
      "3": {
        "value": "1.953rem"
      }
    },

    "title": {
      "value": "{text-size.scale.3}"
    },
    "heading": {
      "value": "{text-size.scale.2}"
    },
    "body-copy": {
      "value": "{text-size.scale.0}"
    }
  },

This reads to me as "this token is not a group, it's a dimension".

I'd like to propose something like...

  "text-size": {
    "contains": "dimension",

    "scale": {
      "0": {
        "value": "1rem"
      },
      "1": {
        "value": "1.25rem"
      },
      "2": {
        "value": "1.563rem"
      },
      "3": {
        "value": "1.953rem"
      }
    },

    "title": {
      "value": "{text-size.scale.3}"
    },
    "heading": {
      "value": "{text-size.scale.2}"
    },
    "body-copy": {
      "value": "{text-size.scale.0}"
    }
  },

It may feel like this is a simple "change the language of type to be groupType in circumstances" sort of proposal, but I architect the tokens file as "group until otherwise denoted", therefore everything is a type of group until it is overridden.

Building a library of the groups of tokens that can be accessed later on for abstraction layers felt helpful in my prototype, plus labeling the group with contains felt to provide "logic" - i.e. every token within this group should inherit a type value from the closest value of contains. This is nice because it can help reduce the file size at scale.

Writing this proposal to avoid overloading the term "type" would seem to be quite helpful for implementors... correct me if I'm wrong too! My proposed modification helped me write more a more reasonable implementation, and craft formatting messages to give structured feedback that had felt reasonable for folks when working with this file during compilation.

But yeah, even adding the ability to do description on a group felt inherently beneficial.

Edit: Updated my language around the "group of tokens" paragraph to improve readability.

splitinfinities commented 3 years ago

A couple other fleeting thoughts I had after I posted the above:

c1rrus commented 2 years ago

Thanks everyone for your comments. There's some interesting points being made and we're planning to discuss them at an upcoming spec editors meeting.

I think a few ideas & concerns have been raised in this thread, so I'll attempt to call out and summarise them here. Please comment if you feel this isn't accurate or I've missed out something.

  1. Is it OK for some group properties to be inherited by child tokens (e.g. type) and others apply to the group itself (e.g. description)
  2. Is it OK for groups to have properties that are inherited by child tokens at all?
    • Concerns have been raised that this might make the format harder to understand and/or parse. Also diffing could become tricky in some situations as the same information could be written in several ways.
  3. Assuming we want groups to be able to set a default type for tokens within that do not set their own explicit type, should we name that group propert something other than type to avoid confusion? (childrenType, tokenProperties.type and contains have all been proposed as alternatives)
kevinmpowell commented 2 years ago

Thanks for the summary @c1rrus

  1. Is it OK for some group properties to be inherited by child tokens (e.g. type) and others apply to the group itself (e.g. description)

I vote "yes", this is okay. I think we can outline this in the spec when we define group properties and denote whether or not each property is inheritable. In my mind it's similar to whether or not javascript events bubble (though that's going from child -> parent).

  1. Is it OK for groups to have properties that are inherited by child tokens at all? Concerns have been raised that this might make the format harder to understand and/or parse. Also diffing could become tricky in some situations as the same information could be written in several ways.

Since one of the key principles of the format is that it be human readable and editable then I think we have to keep the ability for some group properties to be inheritable.

  1. Assuming we want groups to be able to set a default type for tokens within that do not set their own explicit type, should we name that group propert something other than type to avoid confusion? (childrenType, tokenProperties.type and contains have all been proposed as alternatives)

I prefer the simplicity of type so the key name is the same whether it's at the group or token level. If we changed it to childType, contains, defaultType or something else, it'd still mean the same thing. There's not a separate concept of a type at the group level that we need to deconflict with.

c1rrus commented 2 years ago

I've just opened PR #89 to update the spec to move group properties into a metadata object. Thanks to everyone who contributed to this discussion!

dbanksdesign commented 2 years ago

I agree with @kevinmpowell's points. I would put developer experience (reading and editing) over parsing complexity, I think having an inheritable type property will greatly simplify writing design token files by hand. For which properties are inheritable and which aren't, I would think about it the same was as CSS properties and the cascade. Some CSS properties are inherited (color) and others are not (padding).

ChucKN0risK commented 2 years ago

I agree with @dbanksdesign 👍

kevinmpowell commented 2 years ago

Action: Spec to be updated indicating $type at the group level will be inherited by tokens within the group.

Group level $description will not be inherited by tokens and serves as a description for the group.

c1rrus commented 2 years ago

The latest spec draft includes $type and $description properties for groups and clarifies that types are inherited but descriptions are not. Therefore closing this issue.