Polish Card component. #35245
Polish Card component. #35245
Conversation
jasmussen
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +16 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
This doesn't work well when you apply the |
|
Can you elaborate on that? As far as I can tell, the box-shadow/border is removed intentionally and entirely when is-shady is applied: |
|
@jasmussen no it's not removed intentionally, you can try reverting your change and see the result (it's specially not great for CardBody and Footer IMO) |
|
Of course yes, thank you, I see it now:
The alternative I can think of is to change the border-radius value so that it renders as 2px despite being outset. We've done that in some of the SCSS files using Edit: I have a separate question on whether we should have an "is-shady" property for the card component at all, but that's unrelated to your point. |
That is something I was wondering as well. Maybe we could drop support for that entirely (especially for body) but it'd still be great if the border is shown around the "shady" header as this is more common. Components folks should have a better opinion here @ciampo |
|
The reason why the original approach is not working is that the One possible solution is to use a pseudo-element (via Here's the code that I wrote to try this out on my machine (expand to reveal it)diff --git a/packages/components/src/card/styles.js b/packages/components/src/card/styles.js
index 70280ebc72..c8ce8f32b2 100644
--- a/packages/components/src/card/styles.js
+++ b/packages/components/src/card/styles.js
@@ -9,8 +9,20 @@ import { css } from '@emotion/react';
import { COLORS, CONFIG } from '../utils';
export const Card = css`
- box-shadow: inset 0 0 0 1px ${ CONFIG.surfaceBorderColor };
outline: none;
+
+ &::before {
+ content: '';
+
+ box-shadow: inset 0 0 0 1px ${ CONFIG.surfaceBorderColor };
+ position: absolute;
+ top: 0;
+ right: 0;
+ bottom: 0;
+ left: 0;
+
+ pointer-events: none;
+ }
`;
export const Header = css`
@@ -77,7 +89,9 @@ export const borderColor = css`
`;
export const boxShadowless = css`
- box-shadow: none;
+ &::before {
+ content: none;
+ }
`;
export const borderless = css`From a quick look, this seems to apply the changes that @jasmussen was looking for, without the drawbacks of the original approach. Although we'd need to investigate this a bit better to make sure we're not introducing a breaking change.
I'm not sure about the historical reasons for this prop to exists. |
Actually |
Oh ok, thank you for sharing the knowledge! I remember that it is used in WordPress.com and WooCommerce, although I don't know to which extent. |
|
I wonder what an actionable next step on this one is. I wouldn't call it urgent, but it seems like a nice little microcosm of adopting and adapting the components, and therefore a good exercise:
While I imagine a bordered card can be useful, if we find that it isn't, it might be worth simply removing that prop entirely. Just as an example, the ItemGroup is bordered, as are each item inside, and they always will be. This one probably doesn't need any prop to tell it to be bordered. Are we looking at something similar for the card? |
|
I think a good approach here could be to treat each question separately:
What do folks think? |
…t support reading styles from pseudo-elements
ciampo
added
[Feature] UI Components
|
Thanks for sheparding this along! I do wonder if the pseudo element is overding it, though, considering it's just a border. It's not a strong opinion I have — but might there be side effects we don't want? Did we look at the usage of the component and see whether a "bordered" version was even necessary? |
The only side - effect I can think of is if someone was already relying on the
I has a quick look around the repo to see how the component is used:
On top of these usage, I know for sure that this component is currently used in WordPress.com (I believe for the cards used to display the "in-editor" welcome tour) and in some WooCommerce dashboard. There may be more usages that we're not aware of. If we need just a "empty box", we could use the IMO, when I think of a |
|
Good thoughts. I recall @jameskoster mentioning a "Card" component back in the day, if you have time to look, what are your thoughts? Is a "bordered" property a good aspect of a card? What do you see the card accomplishing? Thanks all. |
|
The border property is useful in situations where a card sits on a field that shares the background color. Probably a dumb question, but is there a reason we're using a box-shadow here instead of a simple border? |
The primary reason for me is to keep a consistent and predictable 2px border radius, which works because the shadow can overlay inwards. Of course that can be worked around. The other reason is to make the border color semitransparent, so it picks up color from the background of the card, as shown in the 2nd picture of the PR. I don't have strong opinions in either direction, so long as we land on the right radius. |
That one seems a bit situational. I can see how it works elegantly here, but it might be less desirable in other situations. If anything this feels like it should be a separate property 😬 |
|
The border radius always works, yes, but if I apply The overlaid border is situational, yes, but I've come to be very careful with adding borders as they so quickly end up adding noise, conflicting with input fields and focus styles. And anything we can do to reduce borders, including have them blend better with background colors, is usually a win. But it's not a strong opionin. I like the simplicity of border. We just want to make sure and account for what offset it might add, if any, if a bordered card is to stack next to a non-bordered card, so there isn't a pixel shift. |
When using a shadow, yes. I'm suggesting to use
I 100% agree with this sentiment, but we can't always know the circumstances in which Card will be used. A border is a pretty standard feature of a Card component evidenced by its usage on both WordPress.com and WooCommerce.
We could potentially offset the padding based on the border width? 🤔 |
|
@griffbrad do you remember if there was any technical reason why the |
I don’t believe there is a technical reason, no. @ItsJonQ’s original |
|
Hey @jasmussen , what should the next steps be for this PR? Should we go ahead and test/merge these changes, or should we close it? |
|
I'm a little nervous about using the pseudo selector approach for this, it feels like it's adding some complexity to something that should probably be simple. I think it's fine to shelve/close/pause this PR for now. Even as an exploration, it has been valuable in surfacing some of the needs of the system, compared to what the components offer. For example I remain unconvinced that we need a cardheader and card footer at all — or even just that we don't want border separators between them. And if we were to decide in that way, it might offer us a different path forward to our goal that simplifies things. As is, and looking purely as Gutenberg as the consumer, the Card appears to have been designed in isolation without a clear problem to solve. That's not a critique of the work, but rather a challenge to our assumptions about which components we should have, and how they should be used. |
This makes sense
I definitely share a similar opinion too — not exactly sure why we have card headers / footers / body (although I do think that it's nice to have the possibility to show a border). From what I understand, the components package was initially created by putting together a few "shared" components across Gutenberg. The package lacked an overarching design system, and its components were mostly built out of necessity — which ultimately means that components are not always "consistent" and "coeherent" in terms of design language, public APIs, and also implementation.
Absolutely. The challenge now is to incrementally improve this package without introducing breaking changes — and your feedback has definitely helped a lot already! I'm going to close this PR for now, we can always re-open it in case |
Description
Followup to #35219 (comment). The Card component has a bordered option which paints a drop shadow around it.
This drop shadow is outset, so even though it has the correct 2px border radius applied to it, it appears as a 3px border radius. In addition to this, the fact that it is semitransparent is not visible as the color doesn't overlay the background color inside:
This PR changes that box shadow to be
inset, which both fixes the radius, and makes it overlay the color, and simply darken it instead:How has this been tested?
Open the site editor, open global styles, observe the style card. It's easiest with a theme that has a colored background, such as TT1 blocks.
Checklist:
*.native.jsfiles for terms that need renaming or removal).