-
Notifications
You must be signed in to change notification settings - Fork 4.6k
UI: Add Text component with refined typography tokens #73931
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: trunk
Are you sure you want to change the base?
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| "regular": { | ||
| "$value": 400, | ||
| "$description": "Regular font weight for body text" | ||
| }, | ||
| "medium": { | ||
| "$value": 500, | ||
| "$description": "Medium font weight for emphasis and headings" | ||
| } |
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'm not 100% on these tokens, and would welcome feedback. These are being carried forward from existing variables:
gutenberg/packages/base-styles/_variables.scss
Lines 38 to 40 in ac35a71
| // Weights | |
| $font-weight-regular: 400; | |
| $font-weight-medium: 499; // ensures fallback to 400 (instead of 600) |
Note that I did not use the 499 value here, since this is essentially a browser-specific quirk. As such, it might make sense to apply that transformation as part of the CSS transformation, but the raw value and how it's used elsewhere (e.g. Figma) should be 500.
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.
Being a browser app, isn't it plausible that the same quirk would manifest in Figma though?
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.
Being a browser app, isn't it plausible that the same quirk would manifest in Figma though?
That's a good question. I'm not really sure. Doing some brief inspecting, it looks like the main canvas area in Figma is a <canvas /> element, so I'm not sure if it'd have the same issue.
I think the root of the decision here is codifying the decision that we want 500 as the value in the source design tokens, and dealing with the implementation details as close to the consumption-level as we can manage.
| export const Heading: Story = { | ||
| args: { | ||
| children: 'Heading Text', | ||
| fontFamily: 'heading', | ||
| fontSize: 'xl', | ||
| fontWeight: 'medium', | ||
| lineHeight: 'lg', | ||
| }, | ||
| }; | ||
|
|
||
| export const Body: Story = { | ||
| args: { | ||
| children: 'Body text with regular weight and medium size.', | ||
| fontFamily: 'body', | ||
| fontSize: 'md', | ||
| fontWeight: 'regular', | ||
| lineHeight: 'md', | ||
| }, | ||
| }; |
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.
These combinations did give me pause to consider if this is evidence of the tokens being "primitive", with "semantic" tokens referencing the combinations of those aspects toward application of higher-level concept of "heading" and "body" content.
There's some suggestion to this effect in examples in the DTCG specification: https://www.designtokens.org/tr/2025.10/format/#typography-component-references
My hunch is that the way we're using these values is still semantic, and something we'll need to support. And maybe, on top of that, we could explore pre-composed combinations as a separate set of tokens referencing these.
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 agree they are still semantic. And yes, I think we'll want to offer preset configurations (likely based on these mixins).
Could there be a style property to handle that? It'd allow consumers to use presets, but still override individual aspects when necessary.
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.
Could there be a
styleproperty to handle that? It'd allow consumers to use presets, but still override individual aspects when necessary.
I'd want to avoid style ambiguity with the style property, but we could do something like variant="heading" if that's what you mean, where the variant would apply some precomposed "typography" value in typography.json tokens like...
{
"font": {
"style": {
"heading": {
"$type": "typography",
"$value": {
"fontFamily": { "$ref": "#/font/family/heading/$value" },
"fontWeight": { "$ref": "#/font/weight/medium/$value" },
// ...
}
}
}
}
}Is that what you'd have in mind?
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'd want to avoid style ambiguity with the style property
Derp, of course. Yeah, variant could work. I suppose we'd want presets like body-sm – body-xl and heading-sm – heading-xl. Maybe things like label as well and button.
|
Size Change: +502 B (+0.02%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in ae0645e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20240353070
|
mirka
left a comment
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 would expect this component to support the relevant color tokens as well. What do you think?
| "regular": { | ||
| "$value": 400, | ||
| "$description": "Regular font weight for body text" | ||
| }, | ||
| "medium": { | ||
| "$value": 500, | ||
| "$description": "Medium font weight for emphasis and headings" | ||
| } |
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.
Being a browser app, isn't it plausible that the same quirk would manifest in Figma though?
| /** | ||
| * Default render function that renders a span element with the given props. | ||
| * | ||
| * @param props The props to apply to the HTML element. |
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.
We can probably add this package to this exemption.
Lines 443 to 451 in c8161dd
| files: [ | |
| '**/@(storybook|stories)/**', | |
| 'packages/components/src/**/*.tsx', | |
| ], | |
| rules: { | |
| // Useful to add story descriptions via JSDoc without specifying params, | |
| // or in TypeScript files where params are likely already documented outside of the JSDoc. | |
| 'jsdoc/require-param': 'off', | |
| }, |
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.
We can probably add this package to this exemption.
Interesting. I think it'd follow that we'd want this package to be consistent, yes. And generally I think documentation like this specific example isn't especially useful. I think there could be cases where it'd be useful to include (require) parameter documentation, namely when the parameter isn't a props object. I guess it'd be difficult to differentiate.
Yeah, absolutely 👍 I'll add support for those. |
8d9688d to
ae0645e
Compare
I added this in ae0645e. One thing I'm waffling on is the duplication between how Box handles text color as well. I debated whether One improvement could be to used shared types for available color options, maybe using a Terrazzo plugin to generate the types. I think this would be a good improvement to consider anyways, since trying to manually manage these types is going to be error-prone. I wouldn't be surprised if that's already the case after #73793, for example. |
|
Leaving a quick comment for now, remembering some initial conversation about APIs (example). When I think about how the abstraction that As such, I'd consider:
Apologies in case I missed any context that was shared in the past months while I was actively contributing. |
|
@ciampo Thanks for the feedback and the link to previous discussion context. Regarding variants, I agree this is something we'll want to support, and which should also exist in tokens as preset combinations (they don't yet). There's some discussion of this in #73931 (comment) . I'm aiming for this as a follow-on enhancement. Regarding components and props, I might be misaligned. My perspective on Text and Box at the lowest levels is to provide a bridge to the design tokens in a way that allows consumers to build with components rather than writing their own CSS. As far as maintenance burden, if we view the props as 1-to-1 with design tokens, the burden isn't on which props we support, but rather which tokens we choose to provide, since the props are a reflection of those tokens. |
CSS export will already normalize this, but this is necessary for use in some tools like Figma to ensure correct CSS property code snippets
Co-Authored-By: Lena Morita <[email protected]>
ae0645e to
3bac7fd
Compare
|
I created a separate issue to track the enhancement for supporting presets in #74127.
This was implemented separately in #74080 and leveraged here in 81f8700 and 3bac7fd. I'm not seeing any other explicit blockers in the comments here, so I propose we get this in and iterate. Could someone review this? |
What?
Within
@wordpress/theme, refines typography tokens:kebab-case, ensuring that DevMode code snippet references when imported in Figma reference the correct CSS property names. CSS property names already convert tokebab-casethrough the Terrazzo CSS plugin, so this has no effect on the resulting CSS property name.sm,md,lg(consistent with tokens added in UI: Add Stack component leveraging gap spacing design tokens #73308).weighttokensWithin
@wordpress/ui, implements a newTextcomponent whose purpose is to tie consumer usage to underlying typography design tokens, similar to howBoxis an abstraction to underlying dimension, border, and color tokens.Why?
Testing Instructions
Verify in Storybook examples:
npm run storybook:devScreenshots or screencast