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

Group & file level properties #72

Closed
kevinmpowell opened this issue Sep 14, 2021 · 16 comments
Closed

Group & file level properties #72

kevinmpowell opened this issue Sep 14, 2021 · 16 comments
Labels
dtcg-format All issues related to the format specification.

Comments

@kevinmpowell
Copy link
Contributor

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."
    },
  ...
  }
}
This was referenced Sep 14, 2021
kevinmpowell pushed a commit that referenced this issue Sep 14, 2021
kevinmpowell added a commit that referenced this issue Sep 14, 2021
github-actions bot added a commit that referenced this issue Sep 14, 2021
SHA: d8dc53d
Reason: push, by @kevinmpowell

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@c1rrus
Copy link
Member

c1rrus commented Sep 14, 2021

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:

  • type on a token is explicitly stating what type that token is.
  • type on a group is setting a default type for all tokens within that group (and any nested groups) which applies unless the tokens have their own type property, which then takes precedence.

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"
  }
}

@kevinmpowell kevinmpowell added the dtcg-format All issues related to the format specification. label Sep 21, 2021
@kevinmpowell kevinmpowell changed the title [Format] Group & file level properties Group & file level properties Sep 21, 2021
@firede
Copy link

firede commented Sep 22, 2021

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
Copy link

gossi commented Sep 25, 2021

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:

  • colors.example-token = #AAA
  • dimensions.example-token = 2730px (explanation: 0xAAA = 2730d, pixel is assumed)

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
Copy link

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
Copy link

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:
{
  "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
Copy link

jjcm commented Oct 26, 2021

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
Copy link

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
Copy link

splitinfinities commented Nov 1, 2021

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
Copy link

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

  • it feels like there is a decision here to be made around inheritance for type's, which if this file assumes implicit inheritance, then this may altogether may be a non-issue. The proposal would need to over communicate that for consumers since inheritance is not built into JSON. A similar argument can be made here for description - are we describing the group or passing one value down to the tokens themselves? We need that as a clarifying point. This may be contentious too.

  • if we do not go inheritance, then a signal that isn't type feels necessary. Even if it was plural - types - instead of my idea above of contains, that feels like it would be helpful.

@c1rrus
Copy link
Member

c1rrus commented Dec 13, 2021

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 kevinmpowell added this to the Next Review Cycle milestone Dec 14, 2021
@kevinmpowell
Copy link
Contributor Author

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
Copy link
Member

c1rrus commented Jan 3, 2022

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
Copy link
Member

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
Copy link
Contributor

I agree with @dbanksdesign 👍

@kevinmpowell
Copy link
Contributor Author

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
Copy link
Member

c1rrus commented Jun 13, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dtcg-format All issues related to the format specification.
Projects
None yet
Development

No branches or pull requests

10 participants