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

Errors in "W3C" output #8

Open
c1rrus opened this issue Dec 1, 2022 · 5 comments
Open

Errors in "W3C" output #8

c1rrus opened this issue Dec 1, 2022 · 5 comments

Comments

@c1rrus
Copy link

c1rrus commented Dec 1, 2022

First off, let me quickly say thank you for making this lovely tool. It's great to see new kinds of design token tools appear and, as one of the editors on the DTCG format, I'm also very excited to see tools that support the (draft) DTCG format! 😄

That being said, the output in the "W3C" tab1 does not currently conform to the current DTCG draft specification. Specifically:

  • There is no spacing type, you should output dimension tokens instead.
  • Values for the duration type must be in milliseconds, so "3s" should be converted to "3000ms". Also, the spec does not support the value paused (if you feel it should, you're welcome to propose that in a new issue).
  • There is no easing type, you should output cubicBezier tokens instead. Note that cubicBezier values need to be plain JSON arrays, so "cubic-bezier(0.12, 0, 0.39, 0)" should be converted to [0.12, 0, 0.39, 0].
    • If you want to combine cubic bezier and duration, you can use the transition type.
  • There is no radius type, you should output dimension here as well.
    • Since dimension values can only bepx or rem, the DTCG format doesn't currently have a proper way to represent % values (it probably should though!). You could fall back to using the string type for now, but be aware that type will be removed soon (another reason to add a proper type for percentage values to the spec! 😅).
  • There is no opacity type. The current draft does permit a number type, which you could use for now. Just like string above, that will be removed soon too. However, I see quite a few other tools supporting opacity tokens, so we'll need to add something to the spec to support those. Feedback and ideas welcome!
  • The values of the shadow tokens aren't valid - they need to follow the structure defined in the shadow type section.
    • Right now, the shadow type does not support multiple shadows. Hower, support for that has been proposed and I think it's likely that we'll update the spec to support that. Until then you could potentially split each shadow into a separate token and maybe append a number to the name or something like that (hacky, I know - but that's the best we can do right now).
  • There is no mediaQuery type, you should output dimension tokens there too.
  • The values for fontFamily tokens are probably not quite right. Right now you're outputting the whole font stack as a single string. Syntactically that's valid, but according to the spec tools should interpret that as a single font family name (so you're saying there is 1 font and it is called "Palatino Linotype, serif"). Since these appear to be font stacks, they should be converted into arrays like ["Palation Linotype", "serif"]. See the fontFamily type chapter for details.
  • There are no fontSize or letterSpacing types, you should output dimension tokens there too.
  • There is no lineHeight type, you should output number or dimension tokens there instead (with the caveat that the number type will be going away soon).

Finally, you probably shouldn't add the tokens groups. The current output is like this:

{
  "Color": {
    "$type": "color",
    "tokens": {
      "background-body": {
        "$value": "#233042"
      },
      // more tokens...
    }
  },
  // other groups...
}

However, the name tokens has no special meaning in the spec. So this is interpreted as: There is a group called "Color", which contains a nested group called "tokens", which contains a design token called "background-body". Since tools that convert to code would typically use the path to a token, the corresponding CSS output should be something like --color-tokens-background-body: #233043. That probably wasn't your intent. The solution is to just remove those intermediate tokens groups.

Putting it all together, the corrected "W3C" (DTCG) output should look something like this:

{
  "Color": {
    "$type": "color",
    "background-body": {
      "$value": "#233042"
    },
    "primary-body": {
      "$value": "#354156"
    }
  },
  "Spacing": {
    "$type": "dimension",
    "small": {
      "$value": "4px"
    },
    "medium": {
      "$value": "8px"
    }
  },
  "Duration": {
    "$type": "duration",
    "slow": {
      "$value": "3000ms"
    },
    "fast": {
      "$value": "500ms"
    }
  },
  "Easing": {
    "$type": "cubicBezier",
    "easeInSine": {
      "$value": [0.12, 0, 0.39, 0]
    }
  },
  "Radius": {
    "$type": "dimension",
    "circle": {
      "$type": "string",
      "$value": "50%"
    },
    "large": {
      "$value": "8px"
    },
    "small": {
      "$value": "2px"
    }
  },
  "Opacity": {
    "$type": "number",
    "opacity-25": {
      "$value": 0.25
    },
    "opacity-50": {
      "$value": 0.5
    }
  },
  "Shadow": {
    "$type": "shadow",
    "level-1_1": {
      "$value": {
       " color": "#00000024",
        "offsetX": "0px",
        "offsetY": "1px",
        "blur": "1px",
        "spread": "0px"
      }
    },
    "level-1_2": {
      "$value": {
        "color": "#0000001F",
        "offsetX": "0px",
        "offsetY": "2px",
        "blur": "1px",
        "spread": "-1px"
      }
    },
    "level-1_3": {
      "$value": {
        "color": "#00000033",
        "offsetX": "0px",
        "offsetY": "1px",
        "blur": "3px",
        "spread": "0px"
      }
    }
  },
  "Media Query": {
    "$type": "dimension",
    "max-width-mobile": {
      "$value": "600px"
    }
  },
  "Font Family": {
    "$type": "fontFamily",
    "body": {
      "$value": ["Arial", "Helvetica", "sans-serif"]
    }
  },
  "Font Size": {
    "$type": "dimension",
    "caption": {
      "$value": "12px"
    }
  },
  "Letter Spacing": {
    "$type": "dimension",
    "dense": {
      "$value": "-1px"
    }
  },
  "Line Height": {
    "$type": "number",
    "heading": {
      "$value": "1.25"
    }
  }
}

In principle, I'd be willing to submit PR(s) to fix these things. However, I'm pretty busy at the moment so probably won't have time to contribute fixes anytime soon. I therefore hope you don't mind me raising this issue to at least list out the issues in case someone else would like to try and fix them.

Footnotes

  1. Btw, we would be grateful if you could relabel that to something like "DTCG". While the DTCG is a W3C community group, we are not a W3C working group and our technical reports are not on a W3C standards track. More info here: https://www.designtokens.org/press-kit/

@acapeletto
Copy link
Collaborator

Thank you for the detailed feedback @c1rrus! This is really helpful. We really appreciate the suggestions and comments, the Design Tokens spec was the main inspiration for this tool. We will prioritize these issues for our next weekly release.

Regarding the output tab label, alongside the name change we will add a disclaimer below the code with a link to the DTCG. We definitely don't want to mislead users with the naming.

@c1rrus
Copy link
Author

c1rrus commented Dec 1, 2022

Wow, I wasn't expecting a response so soon. That's amazing! 🙌

Let me know if you have any questions about the format and I'll do my best to help out.

As for the name on the tab, I think "DTCG" is probably fine for now. There isn't really a broad consensus yet on how to refer to the DTCG's file format. We're currently gathering feedback and ideas here: design-tokens/community-group#164 Feel free to make suggestions there!

@apresmoi
Copy link
Collaborator

apresmoi commented Dec 2, 2022

Hello @c1rrus ! As @acapeletto said, thank you for the feedback.

I have a few questions about how you guys see we could align the app "Group types" with the spec's "Token types".

For example, the easing group type, should be able to support any easing-function type.

We currently have something like:

"Easing": {
   "$type": "easing",
   "tokens": {
      "easeInSine": {
         "$value": "cubic-bezier(0.12, 0, 0.39, 0)"
      },
      "easeOutSine": {
         "$value": "cubic-bezier(0.61, 1, 0.88, 1)500ms"
      }
   }
}

wich is wrong, as @c1rrus pointed out. If we are only using cubicBezier functions, then we could do something like so:

"Easing": {
   "$type": "cubicBezier",
   "easeInSine": {
      "$value": [0.12, 0, 0.39, 0]
   },
   "easeOutSine": {
      "$value": [0.61, 1, 0.88, 1]
   }
}

and if we have different timing functions we could add tokens with their specific type

"Easing": {
   "easeInSine": {
      "$type": "cubicBezier",
      "$value": [0.12, 0, 0.39, 0]
   },
   "easeOutSine": {
      "$type": "cubicBezier",
      "$value": [0.61, 1, 0.88, 1]
   },
   "easeLinear": {
      "$type": "string",
      "$value": "linear"
   }
}

I have to review the rest in a little bit more detail.

@c1rrus
Copy link
Author

c1rrus commented Dec 6, 2022

In the current draft we only support Cubic Bézier values. I suppose you could replace CSS keywords like ease-in with their equivalent functions ([0.42, 0, 1, 1]).

However, there's currently no way to represent values like linear or steps(...). You could fall back to using string for now, but it's not ideal as it's meaningless (a bit like <div> in HTML 😜), so other tools can't be expected to know how to use those values (e.g. something like StyleDictionary wouldn't automatically know how to convert that into whatever the Android, iOS, etc. equivalents are).

Feel free to raise an issue or comment on a suitable existing one to propose how we might add support for those kinds of values.

@apresmoi
Copy link
Collaborator

apresmoi commented Dec 7, 2022

Maybe what we have to define is what should be the source of truth for the app @acapeletto.

The user could choose/filter what type of values wants to accept. There could be a dropdown somewhere that lets you pick what type of values you want: Supported by CSS, or Android, or iOS, etc.

Then we should implement inputs with masks/dropdowns depending on the property, etc.

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

No branches or pull requests

3 participants