UI: Implement Box component as part of new UI package#72984
Conversation
aduth
requested review from
ajitbohra,
fabiankaegy and
juanmaguitar
as code owners
|
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. |
| type DimensionVariant< T > = { | ||
| block?: T; | ||
| blockStart?: T; | ||
| blockEnd?: T; | ||
| inline?: T; | ||
| inlineStart?: T; | ||
| inlineEnd?: T; | ||
| }; |
| type BackgroundColor = | ||
| | 'neutral' | ||
| | 'neutral-strong' | ||
| | 'neutral-weak' | ||
| | 'brand' | ||
| | 'success' | ||
| | 'success-weak' | ||
| | 'info' | ||
| | 'info-weak' | ||
| | 'warning' | ||
| | 'warning-weak' | ||
| | 'caution' | ||
| | 'caution-weak' | ||
| | 'error' | ||
| | 'error-weak'; | ||
|
|
||
| type ForegroundColor = | ||
| | 'neutral' | ||
| | 'neutral-weak' | ||
| | 'success' | ||
| | 'success-weak' | ||
| | 'info' | ||
| | 'info-weak' | ||
| | 'warning' | ||
| | 'warning-weak' | ||
| | 'caution' | ||
| | 'caution-weak' | ||
| | 'error' | ||
| | 'error-weak'; |
There was a problem hiding this comment.
In earlier iterations I tried building these up from the color scheme options, but we don't actually support all the combinations within "surface" background or foreground colors. I think that's intentional, but it requires more effort to explicitly list out the tokens which are supported. The way this was implemented also imagines that we might allow for more "families" to be introduced in the future like a "card" family, which would require some revisions to the typings if those other families support different options.
Earlier iteration of color scheme types
type ColorTone =
| "neutral"
| "brand"
| "success"
| "info"
| "warning"
| "caution"
| "error";
type ColorEmphasis = "weak" | "strong";
type ColorState = "active" | "disabled";
type ColorString =
| `${ColorTone}`
| `${ColorTone}-${ColorEmphasis}`
| `${ColorTone}-${ColorEmphasis}-${ColorState}`;
packages/ui/src/box/types.ts
Outdated
| /** | ||
| * The surface background design token for box background color. | ||
| * | ||
| * Shorthand for `background`. | ||
| */ | ||
| bg?: BackgroundColor; | ||
|
|
||
| /** | ||
| * The surface background design token for box background color. | ||
| */ | ||
| background?: BackgroundColor; |
There was a problem hiding this comment.
I'm not fixed to this idea of supporting both short-form and long-form props, but it seems like a nice balance between explicitness and ergonomics for frequent usage. We also use shorthand like "bg" in our tokens, so I wanted to be consistent.
There was a problem hiding this comment.
I’m not fully up to speed with the new UI package, so this may not be an issue, but it might be worth considering how this prop maps to the underlying CSS. In the past, early implementations of the background color block support used the shorthand background property, which later conflicted with the background-image support — especially when both were combined.
If these props are intended to define only the background color, would it make sense to name them more explicitly (e.g. backgroundColor / bgColor) to avoid similar issues down the line?
There was a problem hiding this comment.
That's a great observation @aaronrobertshaw ! I'll put some more thought into this, and at least come up with a plan for if / how we'd reconcile if we needed to expand to cover different aspects of background or text.
There was a problem hiding this comment.
After thinking about this a bit more, I think bg could work as-is:
- The purpose of this component is in providing an integration to the design system tokens, and specifically (in this implementation) color and spacing tokens. I don't see "image" (in
background-image) or most other background-related properties being tokenized in the same way as color. - Even if we needed to expand this, we could support those variations as suffixed properties (e.g.
bgImage) - For context, a motivation for keeping this name as-is is ergonomics for common usage patterns,
<Box bg="brand" />is concise and easy to grok
But I agree in some regards:
- Maybe, like in how similar shorthand is used in e.g. Chakra UI, we should have the long-form variant fully-spelled out as
backgroundColor, or at the very leastbackgroundColoravailable as an alias - We'll want to be clear in documentation that
bgmaps to color. As it stands, the documentation is largely exposed through Storybook stories, which do inherit the TSDoc comment above that references "box background color"
It also got me on a rabbit-hole of thinking of a "clever" solution involving aligning these props one-to-one with the underlying CSSStyleProperties interface, i.e. element.style.backgroundColor = '...' and whether we could have the internal behavior be a more generalized pass-through to style properties with a layer to map values to tokens. This might create some unnecessary complexity or have some conflicts, but I'll plan to do a time-boxed exploration of the idea.
There was a problem hiding this comment.
later conflicted with the background-image support — especially when both were combined.
For visibility, here's the old issue.
One of the main pain points is that gradient values are set with background as well. Not sure when, but we'd really like to fix this one day to support transparent layers.
There was a problem hiding this comment.
- Maybe, like in how similar shorthand is used in e.g. Chakra UI, we should have the long-form variant fully-spelled out as
backgroundColor, or at the very leastbackgroundColoravailable as an alias
In b09519f, I revised the long-form names from background and foreground to backgroundColor and color.
Here's how I'm thinking we can approach this:
- Supported props should align to the CSS property that they control
- Short-hand props should be limited to a few select, very common props
In this way, we could easily expand to add backgroundImage or other styles, and those would not be expected to have short-hand props anyways.
It occurred to me that this is something we'll soon encounter with handling borders, where we'll likely want to support at least both borderColor and borderRadius.
There was a problem hiding this comment.
Coming in late to this, but I think there are a lot of small downsides to having prop aliases, and I'm not sure the benefits will outweigh the downsides here.
- Adds complexity when doing any kind of AST analysis or modification (e.g. lint rules, codemods, prop usage stats).
- Has a learning curve for humans who see the shorthand for the first time.
- Consistency concerns within a single file will subtly add to the cognitive load ("which prop version should I use here?") and affect searchability.
- Adds noise to documentation.
- Subtle bugs when both the canonical prop and alias prop are added mistakenly with different values.
I would also note that "shorthand" as in CSS properties are not aliases, they are shorthands for setting multiple properties at once. CSS would be even more chaotic if it had property aliases.
So I think we should pick a canonical prop name and stick with it. And given that fewer people are typing out their code letter by letter these days, maybe there isn't much benefit to abbreviating them to the shortest possible string.
There was a problem hiding this comment.
I think those are valid points. It's occurred to me as well in reflecting on how the idea with shorthands is often to enable "power users" to have a more expressive APIs, that's not really very considerate of the people who end up maintaining said "power users" code down the line, who may benefit from the more verbose alternative and the consistency of having a consistent way of interacting with these components.
Maybe we can just remove it and evaluate whether it's a problem. We can always revisit it down the line and add it as a backwards-compatible enhancement if we think it'd prove valuable.
|
Size Change: +312 B (+0.01%) Total Size: 2.42 MB
ℹ️ View Unchanged
|
packages/ui/src/box/box.tsx
Outdated
| } | ||
|
|
||
| if ( fg ) { | ||
| style.color = `var(--wpds-color-fg-${ family }-${ fg }, var(--wpds-color-fg-content-${ fg }))`; |
There was a problem hiding this comment.
There is no "surface" family for foreground tokens, but this fallback behavior works nicely for allowing us to define "surface" as the default family, as well as future or custom families that may define some or all of their own tokens.
aduth
added
[Type] Experimental
|
This looks great 😊 |
|
Highlighting a few specific things I'm interested in feedback on:
|
|
|
||
| ### Custom Rendering | ||
|
|
||
| Every component supports the `render` prop for complete control over the underlying HTML element: |
There was a problem hiding this comment.
I'm curious - why do we not utilise React.children here? It still allows for complete flexibility about underlying HTML and follows a more "natural" HTML-like syntax?
There was a problem hiding this comment.
I'm curious - why do we not utilise
React.childrenhere? It still allows for complete flexibility about underlying HTML and follows a more "natural" HTML-like syntax?
The behavior described here is when you want to control the rendering of the wrapper element itself. A good example of this would be if you wanted to render a <Button /> to appear visually like a button but have link semantics, you could do something like <Button render={ <a /> } />. This is similar to what we've done historically with as="a" props (also popular in libraries like Radix), and similar to what other libraries like Ariakit supports.
Many (most?) components would still accept children to render as the descendants of the element.
Curious if you have any suggestions on how to clarify the language here.
There was a problem hiding this comment.
I think render as a propr would almost always suggest to me "this is for controlling the rendering of the inner html" rather than "this is how you control the rendering of the component in question".
If we have a precedent for using as then why not use that again here? It would seem clearer to me.
There was a problem hiding this comment.
For what it's worth, there's precedent of both: The Composite, Menu, and Tabs components all provide a render prop.
The issue with as is that it's pretty limited in its flexibility, and not as compatible with type-checking out of the box.
Using a <Button /> and <a /> example again, we'd ideally want href to be a prop of the link and not the Button, where a render prop allows us to accept props applied to the link without defining them on the Button types.
<Button render={ <a href="/" /> } />
// vs. <Button render="a" href="/" />Regarding flexibility, with this example, you could also imagine someone wanting to use a component abstraction to handle the link behaviors, like a Tanstack Link. If the polymorphism only supports string values, we can't do that.
Some of this we might be able to work around or find some middle-ground (e.g. inferring props using TypeScript generics, allowing as to accept values other than strings).
Another consideration is related to the "Maintenance cost" point raised in #71196, specifically around leveraging third-party libraries to reduce our own maintenance burden. I would expect we may want to increasingly build upon established libraries, and aligning this behavior would make that more seamless. We're already using Ariakit quite a bit, which uses render props. Up-and-coming libraries like Base UI adopt a similar approach for composition.
There was a problem hiding this comment.
Thanks for the clear explanation @aduth. It seems I'm a little behind on this render being an established pattern.
I absolutely agree that we need fully flexibility for this custom rendering. If as only supports strings then it's a no go.
So in summary
renderallows you to customise the element itselfReact.childrenand Composite components pattern allows for full control over inner HTML
There was a problem hiding this comment.
So in summary
renderallows you to customise the element itselfReact.childrenand Composite components pattern allows for full control over inner HTML
Yeah, precisely.
The compound component approach also pairs nicely because you can potentially mix-and-match both first-party and completely custom component combinations, which is difficult to do with monolithic components.
|
|
||
| ### Ref Forwarding | ||
|
|
||
| All components forward refs to their underlying DOM elements: |
There was a problem hiding this comment.
Will there be scenarios where we might wish to pass a ref to more than one underlying element? The scenario I've hit in the past is when you have a form element which has a wrapper div (or something) and then the input itself. You might want access to either of those.
There was a problem hiding this comment.
Will there be scenarios where we might wish to pass a ref to more than one underlying element? The scenario I've hit in the past is when you have a form element which has a wrapper
div(or something) and then theinputitself. You might want access to either of those.
It's not explicitly documented here, but I expect we'll continue to lean heavily on the compound components approach with components in this package, so that in the example you mention, you would apply the ref directly to the rendered subcomponent.
We'll probably want to document a reference to the existing component guidelines and any deviations we'd want to consider.
There was a problem hiding this comment.
...continue to lean heavily on the compound components approach with components in this package
I really appreciate this approach to writing components. The flexibility it affords is huge and it is this kind of thing that will avoid a million and one hacks and an ever expanding list of props.
There was a problem hiding this comment.
In 685fd84, I added a CONTRIBUTING.md to this package which describes how we're inheriting from existing component guidelines, while also reserving space to build upon those with package-specific conventions (for example, folder structure iterations).
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Adds a new @wordpress/ui package with documented "core design principles" for design system components
Overall I'm very much in favour of this new package. A clear design system has been missing for some time and this feels like the first step to rectifying that.
The benefits this could delivery are potentially huge in terms of consistency, maintainability and avoidance of bugs.
| --wpds-spacing-private-50: 40px; /* 10x base spacing */ | ||
| --wpds-spacing-private-60: 48px; /* 12x base spacing */ | ||
| --wpds-spacing-private-70: 56px; /* 14x base spacing */ | ||
| --wpds-spacing-private-80: 64px; /* 16x base spacing */ |
There was a problem hiding this comment.
Is it worth making these explicit calc calls, so that a change to the base spacing will propagate across all variables?
There was a problem hiding this comment.
This file is autogenerated using Terrazzo based on the set of token values, where the actual value is maintained in packages/theme/tokens/spacing.json.
Although your suggestion makes sense and I wondered if we could specify this in the token value itself. Unfortunately from what I found, the DTCG format standard doesn't seem to support this.
If it's any consolation, these values are not intended to be part of a public API or properties that we'd expect anyone to actually use, and as of #72890 they'd not be included in the CSS files at all.
|
For the spacing primitives I think the existing 'grid-unit-*' values are mostly okay, but I'd consider iterating slightly to include increments of 4 up to 24px, then increments of 8 from there. I'd also make 48px the max, because afaik we're not using spacing values any larger than that. I suppose we could also think about width and height. Should we add these as primitives too? E.g.: The example above includes two additional primitive ramps for other dimensions: measure and block. These correspond to the inline and block axes, aligning with CSS logical properties (inline-size and block-size) rather than physical ones. We could alternatively name these groups width and height for immediate familiarity, but the chosen terminology better reflects a logical, direction-agnostic system. Measure would be used for preset surface widths, and block for our preset heights for things like buttons, inputs, and the Editor header. For the semantic surface padding tokens, here are some considerations based on existing UI:
Ultimately I think we may end up needing to iterate on this. Should we incorporate gaps and measure/width here too? E.g.: Maybe “family” is more about "target": surface, interactive, and (optionally) content.
If we introduce content constraints, we could add a small and a semantic like |
|
@jameskoster I appreciate the deep review and feedback for the tokens!
This makes sense to me, particularly if we can keep this an internal implementation detail withheld from the public API 👍
I think probably yes, and certainly something we want to be aware and ensure compatibility with, though also balancing with trying to keep this pull request as narrow in scope as possible to get the package established and an MVP demonstration of the token usage. I fully expect we'll have several more iterations here (radius, typography, etc.) and we'll want to track those follow-on tasks. Do you think width and height could be saved for a follow-on? To the point of compatibility, I think it could make sense to rename For the full example of what you shared with
Here's some action items I'm seeing:
|
Implemented as part of e3969a6.
Implemented as part of e3969a6.
Updated in 7836d9f.
I ended up implementing this here as part of e3969a6, since we're making so many changes to tokens anyways that this diff is unavoidable.
I'm thinking we can keep #72784 open as the tracking issue and include anything there that's missing, likely as a task checklist. With these changes, how do we feel about this as an initial iteration? |
|
Flaky tests detected in 7a71b80. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19268423920
|
|
Looking great, excited about this! :) |
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
This is great work, folks 👍 Let's merge and keep iterating.
aduth
force-pushed
the
add/box-component
branch
from
b09519f to
7a71b80
Compare
|
I updated #72784 to remain open and include a task list of features excluded from this initial pull request. |
8 participants
What?
Adds a new
Boxcomponent as part of a new@wordpress/uipackage, a common primitive component that can be used as a low-level basis for design systems-based components based on shared design standards for e.g. spacing, color, etc.Partially completes #72784 (some features excluded for follow-on)
Related to #71196
Why?
See rationale for this approach in the related issues #72784 and #71196
The included changes also elaborate on the rationale for introducing a new package:
gutenberg/packages/ui/README.md
Lines 5 to 8 in 14179d4
How?
@wordpress/uipackage with documented "core design principles" for design system componentsNote that this is intended to be an early iteration and doesn't fully implement all of the features outlined in #72784
Additionally, there are a number of things I'd like to explore separately for further enhancement, like default "density" integration with ThemeProvider and responsive customization.
Testing Instructions
Verify behavior of Box component in the included Storybook stories.
npm run storybook:devScreenshots or screencast