-
Notifications
You must be signed in to change notification settings - Fork 69
Make whitespace invalid in token names #216
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for dtcg-tr ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This is a major reversal of past choices. |
"$value": 33, | ||
"$type": "number" | ||
}, | ||
"Token cuatro": { | ||
"Token-cuatro": { |
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.
"Token-cuatro": { | |
"token-cuatro": { |
@@ -17,6 +17,8 @@ Note: The `$type` property has been added to ensure this example is valid. Pleas | |||
|
|||
</aside> | |||
|
|||
The name of your design token must not include any whitespace. Otherwise your design token is considered as invalid. |
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.
The name of your design token must not include any whitespace. Otherwise your design token is considered as invalid. | |
Design tokens are written in kebab-case. Otherwise your design token is considered as invalid. |
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.
I think this should be left as is. Kebab-case is only one way of avoiding whitespace... snake_case
or CamelCase
are fine too.
Sorry for pinging an old PR; was just reviewing the open ones. If we want to ban whitespace, should we do something like say adopt CSS’ identifier syntax? Where Of course, this would get weird in JSON where I think
+1 to this question—what problems were we running into where whitespace was the problem? I’m not necessarily opposed to the idea, just lacking context that I can’t find in this PR |
White space makes it harder to write aliases, but aside from that, I'm not sure what the impetus was. @ChucKN0risK can you write down an explainer of the pros and cons for this approach? |
@kaelig & @drwpow from what I recall, (part of?) the intent was to avoid naming clashes when token names are exported as code by translation tools (e.g. Style Dictionary, Terazzo, etc.). In JSON property names are case sensitive and may contain whitespace. So, you could have valid DTCG file like this: {
"My token": { "$value": /* ... */ },
"My token": { "$value": /* ... */ },
"my-token": { "$value": /* ... */ },
" my token ": { "$value": /* ... */ }
} ...and those would be 4 different tokens. However, a lot of programming languages have more restrictions and/or naming conventions for variable names , so when exporting to code, those token names will often end up being kebab-cased, or camelCased, or have some other kind of transform applied to them. Unfortunately, that can sometimes lead to different tokens names that are valid in JSON/DTCG turning into the same output name. E.g. camelCasing the example above, might product the same Also, if I'm not mistaken, in JSON a property name could be entirely whitespace or even an empty string. So this would technically be valid too: {
" ": { "$value": /* ... */ },
"": { "$value": /* ... */ }
} How would you export that to a JS variable? Or, display as a Figma variable name? I think there was a suggestion that perhaps, tools should treat token and group names as case-insensitive and ignore whitespace. So, {
"My token": { "$value": /* ... */ },
"My token": { "$value": /* ... */ },
} ...might have their names normalised by lowercasing and removing whitespace, which could lead to {
"My token": { "$value": /* ... */ },
"alias token 1": { "$value": "{mytoken}" },
"alias token 2": { "$value": "{my-token}" },
} ...should both alias tokens be valid and point to So, I think the idea was, it might be easier to impose limitations on the names of tokens (and groups) in DTCG file to avoid needing to deal with those kinds of edge cases and complexities. @ChucKN0risK and @dbanksdesign may recall more details and be able to provide additional rationale. Personally, I've always been on the fence about this one. Looking at it with fresh eyes now, I do wonder if this is something we can't just leave as is, or perhaps just impose fewer restrictions. Given that there's now quite a few DTCG tools and files out there in the wild, this could be quite a big breaking change for folks. I know the spec is still an editor's draft, but I'd argue it's become a kind de facto standard already, so we should tread carefully. Perhaps names that are only whitespace should be forbidden, since they're difficult to display in a meaningful way in a GUI (e.g. in a design tool) or export to code. I doubt anyone would miss the ability to name a design token There'd still be the potential risk of naming clashes when exporting to code, but I think that's something that export tools could detect and warn/error on (I think Style Dictionary already does this). Also, we could include a recommendation in the spec that file authors avoid choosing sibling token/group names that only differ in case, whitespace or separator characters like |
@c1rrus That all makes sense! Thanks for all the explanation and examples. I agree in providing guidance for these scenarios. Also agree surrounding whitespace should be invalid. I wonder if we should have a more complete identifier specification, similar to CSS ident or something. Rather than incremental changes to disallowed characters. |
Yeah, I agree. Something along the lines of CSS ident would be good. Should we maybe reject this PR and propose something along those lines + guidance in a new PR? |
@kaelig @drwpow I've not been able to find any information in our meeting summaries about this change. But from what you've described @c1rrus I fully agree with you. Let's let token names include whitespaces while preventing a name from just being whitespaces. I can create a new PR along the lines of CSS Indent. Just please confirm me we all reached an agreement here 👍 |
CSS idents do allow any character as long as it is escaped. I assume you mean to constrain token names to what is valid in CSS idents without any escaping. So |
I'd propose instead of standardizing on CSS idents in this case, we use the JSON spec (specifically, strings, which are the only allowed type in a key) as a foundation for token names; there are valid css idents that are not valid JSON strings*, so if a dtcg file was evaluated as json then some funky things might happen. * |
+1 on this one. As far as generating and managing DTCG file by humans, simple rules for managing spaces has great value. However, the token name is an identifier that has a far reaching significance for tooling. Unlike CSS , which targets one target system (CSS parsers), the token name for Design token act as an identifier not just within the JSON document that represents Token (or list of tokens), but via translation tool with other ecosystems as well (It could be a CSS variable, Class name, Attribute name, ...). It would be great usability improvement, if a token name is still "identifiable uniquely" after it is translated. This goes a long way in developing reliable tooling and predictable behavior for references and aliases. Which in turn, makes human life easier when debugging design token related issues in target implementation. Proposoal, Following is modified CSS spec, after dropping support for escaped character. |
@d34dman hm, so, with that spec, we would allow emojis? 🤔 we could look at the "key" in JS objects as a leading guide here IMHO. Or am I way off? |
The idea was not about allowing emojis, but to maintain the support for non latin scripts. Emoji support was a side-effect of that.
The proposal is different in a way that, we specify an allowed list of characters for token, instead of a dis-allow list. JS Object's key are strings right? So then, we will need to dis-allow some characters like [".", "{", "}"], and make the list evolve into spaces and some more We might be better off by declaring which values are valid, instead of keeping a list of invalid characters. This way we can introduce more control characters if needed, with backward compatibility. E.g. A different reference format using characters like "#" and "/" as discussed here #259 or adding more control characters like "+" or "&" for predictable mixing and composing of tokens. At this point this is mere speculation though. |
I'm still a fan of the JSON definition of a string, as it is:
The upside is that we can say a DTS file must be valid JSON. On the downside, it does mean that whitespace is valid in token names. We can follow up on what is syntactically valid vs. recommended/not recommended as a non-normative thing. |
We've decided to consider a token as invalid when its name includes whitespaces.
design-token.md
file