-
Notifications
You must be signed in to change notification settings - Fork 4.6k
UI: Add border support to Box component #73530
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
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. |
|
Size Change: +190 B (+0.01%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a0c4aed. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19831554510
|
juanfra
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.
Thanks for the PR! LGTM ✅
box-border.mp4
897a43b to
12b285b
Compare
| if ( borderColor ) { | ||
| style.borderColor = `var(--wpds-color-stroke-${ target }-${ borderColor }, var(--wpds-color-stroke-surface-${ borderColor }))`; | ||
| } |
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.
One thought I had (which we can iterate on asynchronously): Should any of these border props influence each other, or is it better for them to be independent. For example, if I do <Box borderColor="brand" />, should we default a 1px width since otherwise it doesn't really do anything? I think the upside is that it's a little more ergonomic and infers some intention, with the downside being that it may not be as predictable for certain defaulting to apply only under some circumstances.
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.
Unless I’m misunderstanding something, my preference would be for them to be independent. I know this can be subjective. Once props start behaving in an opinionated way, things can get less predictable and potentially confusing for folks using the component.
That said, I'm not opposed to having some interplay between them if we feel it meaningfully improves things, as long as it’s clearly documented and we call out all the specific cases.
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 your feedback @juanfra . I'd tend to agree. It's also something we could revisit down the line as a future enhancement if it proves to be too burdensome as-is.
What?
Addresses part of #72784
Updates the Box component added in #72984 to support additional border integration with design tokens.
Includes a few supporting changes to align border tokens to general structure and naming scheme.
Why?
As a foundational component, Box should be able to support the most common integrations with design tokens, including border tokens.
Testing Instructions
Verify in Storybook:
npm run storybook:devborderColor,borderRadius, andborderWidthpropsScreenshots or screencast