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

Proposal: changes to type: $dimension and type: $duration tokens #244

Closed drwpow closed 4 weeks ago

drwpow commented 3 months ago

Summary

This proposes the following changes to $type: dimension and $type: duration tokens, which have the same designs:

Reasoning

This PR proposes that the following discussions be resolved like so:

Pros

Cons

Alternatives

Notes

(PR description edited from original version to reflect changes in code)

netlify[bot] commented 3 months ago

Deploy Preview for dtcg-tr ready!

Name Link
Latest commit b7f8502eb50186850981fbb2fa4f3fe1fab2804c
Latest deploy log https://app.netlify.com/sites/dtcg-tr/deploys/671a9df6504491000860a6c4
Deploy Preview https://deploy-preview-244--dtcg-tr.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dev-nicolaos commented 3 months ago

I like this proposal, it seems likely to make parsing easier for tools without introducing much burden on those who'd prefer to write tokens by hand.

Making dimension units unrestricted seems like a win for platform agnosticism too. The web has so many dimension units and this lets token maintainers take advantage of that without introducing a bunch of web specific stuff to the spec.

I wonder if the value property names couldn't use a little bikeshedding but, I can't think of anything better right now.

romainmenke commented 3 months ago

Thank you for this proposal @drwpow! Overall this looks good to me.


The original proposal had $value.number instead of $value.value, possibly to reduce duplication but I didn’t find a rationale. This changes to value because that’s a more common nomenclature for this term,

Yes, $value.value is better!


Dimensions: no restrictions on what $value.unit can be (author-defined)

We can't have unrestricted units.

We will end up having these issues:

Instead we should have a fixed set of specified units and allow any other unit as long as it is vendor prefixed.

Then there can't be conflicts between tools as those would be different vendors. There also can't be conflicts between tools and future spec expansion.

It still leaves plenty of room for custom definitions.

Keep in mind that design token files are a carrier format. How a unit is encoded in a file doesn't dictate how it is exposed in a design tool or the final output for developers.

Figma can create a custom unit y that is exposed to designers as y and output to developers as y while encoding it as figma-y in token files.

If a custom, vendor prefixed, unit is popular and gains adoption among other tools it is also a clear signal that it is useful. Such a custom unit should then go through the spec process to standardize it.

Instead of figma-y as the encoded unit it could also be a separate prop.

{
  "value": 10,
  "unit": "y",
  "vendor": "figma"
}
drwpow commented 3 months ago

@romainmenke Those are all great points, thanks for raising. I was originally trying to solve for multiple things here, but agree that may be too big a change here:

But you address those nicely with the vendor addition proposal. I really like this version specifically for the following reasons:

{
  "value": 10,
  "unit": "y",
  "vendor": "figma"
}

Minor point: I would assume that if the following were provided:

{
  "value": 10,
  "unit": "px",
  "vendor": "foobar"
}

This would NOT be interpreted as px as-defined by the spec. Because if, say, the intent was that it’s parsed as a px dimension by tooling by default, but a particular tool wants to convert it differently, then that’s the case where $extensions should be used instead (all tooling operates in a standard way, but in one particular tool, additional metadata overrides behavior). So am I correct in understanding unit + vendor = a unique unit that has no relation to any other?

drwpow commented 3 months ago

But all that said, I’ll make this PR easier to review by NOT adding vendor here, and we could review that in a followup. I’ll restore the current behavior of only allowing px and rem and keeping those as currently-defined. Will also update the PR + description.

🤔 I think this still rejects #218 because that proposes numbers-only values.

romainmenke commented 3 months ago

So am I correct in understanding unit + vendor = a unique unit that has no relation to any other?

Yes :)

I’ll make this PR easier to review by NOT adding vendor here

I agree.

This will also give people the time to think about something like that and give feedback.

c1rrus commented 3 months ago

Btw, i agree with the earlier point from @romainmenke about keeping a defined set of allowed units. Without that I fear we'd quickly end up in a world of incompatible tools and files due different folks supporting different units, or interpreting the same unit in different ways.

I am intrigued and potentially in favour of adding support for custom units though. But, agree that should be done separate from this PR.

drwpow commented 3 months ago

Making parsing easier is nice, but I'm not convinced the current string values are a significant blocker or source of bugs for implementors.

I was of this opinion until rereading #121. There wasn’t a specific comment that touched on this, but comments like @jonathantneal’s reminded me that parsing can be trickier than it seems.

In most languages, validating it isn’t too bad (can be done with RegEx). But when you need to separate it into number and unit, it is trickier than it seems at first glance. In JS, for one, you have parseFloat() and parseInt(), and one may parse as an integer when it’s not. But then you also have BigInt! And don’t forget negative numbers!

For other languages (that aren’t JS) it gets harder. For example, in Rust, Serde is a popular library for interpreting JSON into Rust data structures and can automate a lot of the mapping and memory management. It gets trickier when you’re storing values that are delivered as strings, but they ultimately want to be numbers paired with enums. The difference goes from anyone using an off-the-shelf library to traverse tokens JSON, to having to deal with parsing issues. It’s within the realm of tooling authors, sure, but I think it raises the bar for most devs who haven’t dealt with this extensively.

And of course that’s a shared problem among all languages: storing those numbers and units somewhere. After parsing, now your tooling has to have additional scaffolding to store those saved numbers and units separately from the original token JSON, and keep the mapping between the two preserved.

I think in hindsight, offloading all that work onto tooling doesn’t make sense, when even CSS ASTs like csstree separate values & units so they can be validated independently. It works when we’re plugging tokens straight into CSS, but for any other platform and language, it requires a lot more work than I think we originally realized.

kevinmpowell commented 3 months ago

Any ideas on how this would support unitless line-height? Would the unit just be an empty string in that case?

romainmenke commented 3 months ago

Any ideas on how this would support unitless line-height? Would the unit just be an empty string in that case?

That is just a number, right? Not a dimension.

I don't think it is needed to add dimensions with a "null unit" as a special form of number when regular numbers exist :)

ChucKN0risK commented 2 months ago

Thanks for creating this PR @drwpow 👍

I think splitting the value to an object format is the best solution. I still easy to read while it makes it easier to parse for other languages and tools as you explained.

I also think we should create a new PR to introduce the vendor property.

And finally I agree with @romainmenke about the fact we don't need to add dimensions with a "null unit". Unitless line height can be created as number tokens.

drwpow commented 1 month ago

DTCG meeting just met and this proposal has been approved! 🎉. Will update this PR with all the suggested changes