-
Notifications
You must be signed in to change notification settings - Fork 63
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
Proposal: changes to type: $dimension and type: $duration tokens #244
Conversation
✅ Deploy Preview for dtcg-tr ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
"$type": "dimension" | ||
} | ||
} | ||
``` | ||
|
||
</aside> | ||
|
||
The "px" and "rem" units are to be interpreted the same way they are in CSS: | ||
### Validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this new subheading is inconsistent with the other sections. This was done thinking back to #200 that observes the spec could be more explicit about validation.
This would be a change from the current structure of validation rules existing in prose, usually in the initial description. However, in many places, those aren’t exhaustive/complete (for good reason—it’d be hard to read).
All that said, we don’t have to introduce these subheadings in this PR; we could fold everything into prose form to match the existing format.
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 |
Thank you for this proposal @drwpow!
Yes,
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. It still leaves plenty of room for custom definitions. Keep in mind that design token files are a carrier format. Figma can create a custom unit 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 {
"value": 10,
"unit": "y",
"vendor": "figma"
} |
@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 {
"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 |
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 🤔 I think this still rejects #218 because that proposes numbers-only values. |
Yes :)
I agree. This will also give people the time to think about something like that and give feedback. |
a04d09a
to
5dac755
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR @drwpow. It looks good. I spotted a few minor inconsistencies though - see the other review comments.
However, I'm on the fence about whether or not dimension and duration values need to be changed to an object format. Especially since it's a breaking change (i.e. files and tools written the current draft spec will need to be updated if this goes in). Not a deal breaker since we're still in "editor's draft" territory, but also not something to take lightly given that there are a number of DTCG tools and files out there in the wild.
Making parsing easier is nice, but I'm not convinced the current string values are a significant blocker or source of bugs for implementors. The more significant improvement if this were to go in is more consistency within the DTCG format's syntax. Especially since color values are also due to change to an object structure in order to support more color spaces and wider gamuts. I think aligning all type's value syntaxes around some JSON-esque rather than having some encode multiple pieces of info into a string and others not.
As you've noted this does make things more verbose for humans wanting to read or hand-edit DTCG files. However, I think the proposed syntax is still understandable. Also, I guess this would make creating a JSON schema for the DTCG format easier, and thus also make it easier auto-complete suggestions in editors like VSCode. So on balance, I think it could be a worthwhile trade off.
Nonetheless, I'd like some other format editors to take a look and chime in with their thoughts before I approve this PR.
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. |
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 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 |
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 I don't think it is needed to add dimensions with a "null unit" as a special form of number when regular numbers exist :) |
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 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 |
DTCG meeting just met and this proposal has been approved! 🎉. Will update this PR with all the suggested changes |
896be91
to
f95cc4d
Compare
SHA: b67cfd3 Reason: push, by drwpow Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
This proposes the following changes to
$type: dimension
and$type: duration
tokens, which have the same designs:$value
from a string → object withvalue
andunit
keys for both dimension and durationReasoning
This PR proposes that the following discussions be resolved like so:
Pros
.js
, they could normalize everything topx
and just output numbers (which is the default behavior of JS styling). Tooling wouldn’t have to try and parse the string from the number, potentially introducing bugs.Cons
Alternatives
$value.unit
for dimensions, however, that seems to introduce more problems and regress that part of the schema (see comment below)px
values and durations intoms
. But this would break many setups in tokens (chiefly, web—rem
s andpx
are distinct values with different uses, and based on implementation, aren’t always mathematically-equivalent).Notes
The original proposal had
$value.number
instead of$value.value
, possibly to reduce duplication but I didn’t find a rationale. This changes tovalue
because that’s a more common nomenclature for this term, e.g.csstree
:(PR description edited from original version to reflect changes in code)