Repository navigation
[Draft] font shorthand and full typography token support - #479
michaelurban wants to merge 8 commits into
Conversation
* token-tools: Added support for font shorthand * plugin-sass: Rewrote plugin and added support for font shorthand * plugin-css: Updated `generateShorthand` args * tokens: Added initial test tokens (will be removed before PR)
|
| --tz-ease-quint-in: cubic-bezier(0.64, 0, 0.78, 0); | ||
| --tz-ease-quint-in-out: cubic-bezier(0.83, 0, 0.17, 1); | ||
| --tz-ease-quint-out: cubic-bezier(0.22, 1, 0.36, 1); | ||
| --tz-font-body: var(--tz-font-body-font-weight) var(--tz-font-body-font-size)/var(--tz-font-body-line-height) var(--tz-font-body-font-family); |
There was a problem hiding this comment.
I know it’s still in-progress, but leaving a note for now: we do need to require font-style to be declared, too, to set this property. Otherwise you’re blowing away other cascading token values.
I know one could argue it’s a new feature therefore it doesn’t affect previous users, but I’m requiring this because it will affect future users who can and will use this, and will get bitten by it. Especially with upcoming improvements to modes with the Token Resolver spec.
There was a problem hiding this comment.
font-style is supported. Look at the tokens, font.body doesn't define a font-style.
font.body:
"body": {
"$value": {
"fontFamily": "{font.sans}",
"fontVariationSettings": "\"wght\" 400",
"fontWeight": 400,
"fontSize": { "value": 0.875, "unit": "rem" },
"lineHeight": 1.4375
}
}
font.mtest-full does:
"mtest-full": {
"$value": {
"fontFamily": "{font.sans}",
"fontSize": "13px",
"lineHeight": 1.2,
"fontWeight": 900,
"fontStyle": "italic",
"fontVariant": "small-caps"
}
}
--tz-font-mtest-full: var(--tz-font-mtest-full-font-style) var(--tz-font-mtest-full-font-variant) var(--tz-font-mtest-full-font-weight) var(--tz-font-mtest-full-font-size)/var(--tz-font-mtest-full-line-height) var(--tz-font-mtest-full-font-family);
--tz-font-mtest-full-font-family: var(--tz-font-sans);
--tz-font-mtest-full-font-size: 13px;
--tz-font-mtest-full-font-style: italic;
--tz-font-mtest-full-font-variant: small-caps;
--tz-font-mtest-full-font-weight: 900;
--tz-font-mtest-full-line-height: 1.2;
There was a problem hiding this comment.
The only property that's technically part of the shorthand that isn't supported by this change is font-stretch.. It's deprecated.
There was a problem hiding this comment.
font-styleis supported.
Right, but I’m saying something different: we should not generate this shorthand unless ALL properties exist:
But agree on font-stretch—I’d be willing to just pretend that doesn’t matter. But in this example, I’m saying because tz-font-body doesn’t define font-style, the shorthand shouldn’t even be generated at all because it’s overriding a value it doesn’t intend to.
There was a problem hiding this comment.
–or— alternatively, what if we did something like:
- --tz-font-body: var(--tz-font-body-font-weight) var(--tz-font-body-font-size)/var(--tz-font-body-line-height) var(--tz-font-body-font-family);
+ --tz-font-body: var(--tz-font-body-font-style) var(--tz-font-body-font-variant) var(--tz-font-body-font-weight) var(--tz-font-body-font-size)/var(--tz-font-body-line-height) var(--tz-font-body-font-family);
--tz-font-body-font-family: var(--tz-font-sans);
--tz-font-body-font-size: 0.875rem;
+ --tz-font-body-font-style: inherit;
+ --tz-font-body-font-variant: inherit;
--tz-font-body-font-variation-settings: "wght" 400;
--tz-font-body-font-weight: 400;
--tz-font-body-line-height: 1.4375;I’d be more OK with this—this now correctly inherits from other modes and the cascade propertly. And is backwards-compatible
There was a problem hiding this comment.
It's exceedingly rare that you'll ever want to specify all of the bits of the shorthand and there are reasons why you'd want to use it without doing so.
I do see where you're coming from, truly. And, If this were a clean slate project I'd continue to strongly argue that the font shorthand should be the default and that exploded font properties should be the exception. I mean, if the font shorthand is being triggered it means that you're setting the font family. That's huge. There's a reason why the font shorthand font-style defaults to normal instead of inherit.
"Don't break user space": tz has been loose historically, so don't break user space.
However, if you have a chance to make a breaking change, I'd argue for this one.
--tz-font-body: var(--tz-font-body-font-style) var(--tz-font-body-font-variant) var(--tz-font-body-font-weight) var(--tz-font-body-font-size)/var(--tz-font-body-line-height) var(--tz-font-body-font-family);
--tz-font-body-font-style: inherit;
--tz-font-body-font-variant: inherit;
...
I don't like the idea of adding two additional variables to every definition to accommodate the shorthand. And it's basically creating a new default behavior for the font shorthand, which will get confusing.
Proposal:
In the sass plugin:
-
User space is currently broken. The sass that ships with the sass plugin is invalid. If the font shorthand is going to be a larger/longer conversation let's split out the sass fixes and get them applied.
-
Allow users to access all of the typography variables. This includes the font shorthand where possible and returning
__tzerrorwhen it's not. -
Keep typography mixin's default "exploded font properties" behavior.
-
Add a new mixin (or a flag to the typography mixin) that allows users to include the font shorthand and any extra properties that aren't defined in the shorthand. If you need the shorthand, you know why you need it. Again, there's a reason why the font shorthand
font-styledefaults tonormalinstead ofinherit.
I'm not intimately familiar with the Token Resolver spec. As long as you're able to see that font-style has been specified in one mode you'd be able to emit the correct css var and add a reference to it in the shorthand for all of the modes. Nothing would have to change if that's the case.
There was a problem hiding this comment.
The font shorthand is the default approach in style dictionary. But there's also a fundamental difference of approach between tz and sd. In tz typography is a mixin, in sd it's just a variable with the shorthand.
I think it should be a mixin, but that the shorthand should default syntax used if both font-size and font-family are present.
There was a problem hiding this comment.
Thanks for all those great thoughts! I originally thought I had clarity, but now I want to chew on all your points and think through it more holistically. I do agree that typography tokens in CSS have been a pain point, and I’d like to make it better.
One point of drift between Terrazzo and the DTCG spec is we’ve been lax about enforcing all properties are set (test), whereas the spec requires all properties. The more I think about it I’m starting to agree that when setting a typography token, perhaps resetting all properties actually is desirable rather than deal with an unpredictable cascade of whatever-the-hell is happening in the parent scope.
I don’t know if I have a clear direction yet, just TL;DR “thinking about it.” But I think we’re both thinking about it the same way 🙂
There was a problem hiding this comment.
OK thanks for letting me think about it. I’d be OK with making the following breaking change in both CSS and Sass plugins:
Warning
Partial $type: typography tokens no longer inherit properties from parents. Whenever you use $type: typography, expect font-weight, font-style, etc. to be reset back to the default values if not specified in the token. To inherit properties, use $type: fontWeight and $type: fontFamily tokens instead.
Which I think is the exact behavior you’re describing. This should also align more closely with the W3C specification definition of the typography token too—when you get into cascading and partial properties, it becomes unmanageable to the point it’s unusable.
Thanks for talking this through!
There was a problem hiding this comment.
So to add, more clearly, we don’t need to add more tokens that just declare default values. In fact, that may just introduce weirdness because it’s up to browsers and user stylesheets, etc. So I think we’re sticking with your original proposal, without changes? Unless I’m missing something
| import { defineConfig } from '@terrazzo/cli'; | ||
| import { makeCSSVar } from '@terrazzo/token-tools/css'; | ||
| import css from '@terrazzo/plugin-css'; | ||
| import sass from '@terrazzo/plugin-sass'; |
There was a problem hiding this comment.
Note: this is fine for locally-testing, but all tests should be in plugin-sass and this package shouldn’t be modified at all. But feel free to copy examples from this into the plugin-sass tests just for testing
There was a problem hiding this comment.
There for quick testing only, will be removed in the final PR
|
Shipped in #589 🙂. Kept approach as-is, just made some insignificant style changes to closer match some existing code. Will still credit you in the changelog. Thank you! |
Draft PR. No tests or guarantees included. If it looks like the right direction I'll clean it up and add tests. Updated draft version includes full typography token support in the
sass-plugin