Skip to content

Card Component Padding System Enhancement#72511

Merged
gigitux merged 25 commits intotrunkfrom
72175-card-support-more-padding-options
Nov 4, 2025
Merged

Card Component Padding System Enhancement#72511
gigitux merged 25 commits intotrunkfrom
72175-card-support-more-padding-options

Conversation

@gigitux
Copy link
Copy Markdown
Contributor

@gigitux gigitux commented Oct 21, 2025

What?

Closes #72175

This PR enhances the Card component's padding system by introducing directional padding support and allowing independent padding configuration for Card, CardHeader, and CardBody components.

Why?

The current Card component's padding system only supports predefined token sizes applied uniformly to all sides. This limits the component's flexibility in complex layouts where different padding values are needed for different sides or subcomponents.

This enhancement provides more granular control over padding while maintaining the simplicity of the existing token-based system.

How?

  1. Enhances the size prop to accept either a token or a directional object.
  2. CardHeader and CardBody components have the size prop.

Testing Instructions

  1. Open the Card component storybook.
  2. See the Padding variation story.

@gigitux gigitux linked an issue Oct 21, 2025 that may be closed by this pull request
@gigitux gigitux force-pushed the 72175-card-support-more-padding-options branch from f0895d6 to 35212ec Compare October 21, 2025 09:01
@gigitux gigitux force-pushed the 72175-card-support-more-padding-options branch from 32b730e to 9351230 Compare October 21, 2025 17:39
@gigitux gigitux self-assigned this Oct 22, 2025
@gigitux gigitux added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system labels Oct 22, 2025
@gigitux gigitux marked this pull request as ready for review October 22, 2025 12:10
@gigitux gigitux requested a review from ajitbohra as a code owner October 22, 2025 12:10
@gigitux gigitux requested a review from mirka October 22, 2025 12:11
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 22, 2025

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: gigitux <gigitux@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: aduth <aduth@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@gigitux gigitux requested a review from jameskoster October 22, 2025 12:11
@mirka mirka requested a review from a team October 23, 2025 09:07
Copy link
Copy Markdown
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks for the enhancement.

Could you also update the readme files? The Card component family isn't hooked up to our readme auto-generation system yet, sorry.

Comment on lines +83 to +105
@@ -90,8 +98,8 @@ type MarginalSubComponentProps = BaseSubComponentProps & {
isBorderless?: boolean;
};

export type HeaderProps = MarginalSubComponentProps;
export type HeaderProps = MarginalSubComponentProps & SizeableProps;

export type FooterProps = MarginalSubComponentProps & {
justify?: CSSProperties[ 'justifyContent' ];
};
} & SizeableProps;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these additions necessary? It looks like SizeableProps is already included through BaseSubComponentProps.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 832e1b1

* Internal dependencies
*/
import type { Props, SizeToken } from './types';
import { cardPaddings } from './styles';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the cardPaddings stuff directly into this file? They're not used anywhere else. While you're at it, could you also move all the card padding values out of the config-values file? We want to get rid of that file eventually. I think this get-padding-by-size.ts file can be the canonical source of card padding sizes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with f5d8c47

Comment on lines +95 to +102
/**
* The Card component supports three approaches to padding:
* 1. Default padding (medium) - no size prop needed
* 2. Token-based padding - using size tokens: xSmall (8px), small (16px), medium (24px), large (32px)
* 3. Directional padding - customize each side independently
*
* Each component (Card, CardHeader, CardBody) can have its own padding configuration.
*/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we remove the exact pixel values from this story description and from the code comments? We don't want to commit to pixel values in documentation, since they could be changed at some point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with c4af856

@mirka mirka added [Package] Components /packages/components and removed [Feature] UI Components Impacts or related to the UI component system labels Oct 24, 2025
@gigitux
Copy link
Copy Markdown
Contributor Author

gigitux commented Oct 24, 2025

Looking good, thanks for the enhancement.

Could you also update the readme files? The Card component family isn't hooked up to our readme auto-generation system yet, sorry.

Thanks for the review! I addressed your feedback. Also, I updated the readme too: 9768cee

@gigitux gigitux requested a review from mirka October 24, 2025 08:47
@gigitux
Copy link
Copy Markdown
Contributor Author

gigitux commented Oct 29, 2025

@mirka can you take another look at this PR? Thanks!

Copy link
Copy Markdown
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be out next week, so I'll leave it to my pals on @WordPress/gutenberg-components for any follow up 🙏

Comment on lines +67 to +72
return css`
padding: ${ getSinglePaddingValue( top ) }
${ getSinglePaddingValue( right ) }
${ getSinglePaddingValue( bottom ) }
${ getSinglePaddingValue( left ) };
`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The padding shorthand property is not a logical one, so it's not going to respect our logical values. We have to set each property separately (padding-block-start, etc).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry! I completely forgot to update it! I fixed it!

Comment on lines +49 to +51
default:
return '0';
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what I think about the "fall back to 0" behavior, given that 0 is not an explicitly allowed value and the object shape marks all properties as optional. I can see it becoming a weird loophole with consumers who want to set a 0 padding.

	| {
			blockStart?: SizeToken;
			blockEnd?: SizeToken;
			inlineStart?: SizeToken;
			inlineEnd?: SizeToken;
	  };

I can think of two ways to address this:

  1. Mark all sides as required in the object case, and fall back to medium for undefined or invalid sides.
  2. Officially support none as a size token.

No strong option here — 2 is probably fine? I'll also note that there's already a CardMedia subcomponent that can be used for full-bleed content.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark all sides as required in the object case, and fall back to medium for undefined or invalid sides.

I took this approach given that it is simple.

gigitux and others added 2 commits October 30, 2025 16:25
@aduth
Copy link
Copy Markdown
Member

aduth commented Oct 30, 2025

Not to detract from the effort here, but it occurred to me that a low-level primitive for containers (with support for properties like padding) might be a useful addition to the design system. It could be used to compose things like Cards, but also many other 'containers' like badges, notices, panels, popovers... you name it :D

I created an issue for this at #72784 . The theme tokens for spacing are very incomplete so I wouldn't recommend basing anything off that quite yet, but we'll want to be mindful to potential future compatibility to align the direction here with ideas we want to explore there, so that we could ideally refactor what we're introducing here to use those primitives.

I think the current direction here of an object of logical properties is one approach we could consider for this kind of direction-specific padding customization on a box primitive. I think the size values might be something we'd want to standardize at a design systems level, but considering that the proposed changes builds on existing values for Card size, I don't think that's an issue to sort out here.

@gigitux gigitux requested review from aduth and mirka October 30, 2025 15:46
Copy link
Copy Markdown
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few minor comments, but this looks good 👍 Of my comments, the most impactful is the one about differences between horizontal and vertical spacing with values like 'medium'.

Comment on lines +48 to +49
case 'medium':
return space( 6 );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quirk of this approach is that if I specify size={ { blockStart: 'medium' } }, I'd get space( 6 ) for the top padding, but if I specify size="medium", I get space( 4 ) for the top padding.

We could maybe have some convoluted logic for mapping sizes based on vertical or horizontal properties, but I do quite like keeping this simpler with each step mapping to 2, 4, 6, and 8.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, do you expect some changes here, or is it just a note?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily expecting any changes, just flagging a potential point of confusion or inconsistency of expectations. Unless you have concerns about it, I'm personally happy to try this as-is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it could cause some confusion, but I’m not sure how to fix it without introducing breaking changes.

The idea is that with size={{ blockStart: 'medium', blockEnd: 'medium', inlineStart: 'medium', inlineEnd: 'medium' }}, we can define consistent padding across all properties.

}

// Handle object-based sizes
if ( size && typeof size === 'object' ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the typeof here check redundant? I can appreciate the explicitness, and only mention it because checking object-ness is pretty fraught (see also isPlainObject in various utility libraries), e.g. typeof null === 'object'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed with 767da77


// Handle object-based sizes
if ( size && typeof size === 'object' ) {
const top = size.blockStart;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd probably avoid the top, etc. naming because the idea of logical properties is to avoid associating with specific directions like this, even if it's more familiar due to historical usage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed with 767da77

Comment on lines +12 to +40
const CONFIG = {
cardPaddingXSmall: `${ space( 2 ) }`,
cardPaddingSmall: `${ space( 4 ) }`,
cardPaddingMedium: `${ space( 4 ) } ${ space( 6 ) }`,
cardPaddingLarge: `${ space( 6 ) } ${ space( 8 ) }`,
};

const xSmallCardPadding = css`
padding: ${ CONFIG.cardPaddingXSmall };
`;

export const cardPaddings = {
none: css`
padding: 0;
`,
large: css`
padding: ${ CONFIG.cardPaddingLarge };
`,
medium: css`
padding: ${ CONFIG.cardPaddingMedium };
`,
small: css`
padding: ${ CONFIG.cardPaddingSmall };
`,
xSmall: xSmallCardPadding,
// The `extraSmall` size is not officially documented, but the following styles
// are kept for legacy reasons to support older values of the `size` prop.
extraSmall: xSmallCardPadding,
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cardPaddings is already essentially acting like a constant, so internally referencing CONFIG seems like unnecessary indirection over just inlining the values.

Suggested change
const CONFIG = {
cardPaddingXSmall: `${ space( 2 ) }`,
cardPaddingSmall: `${ space( 4 ) }`,
cardPaddingMedium: `${ space( 4 ) } ${ space( 6 ) }`,
cardPaddingLarge: `${ space( 6 ) } ${ space( 8 ) }`,
};
const xSmallCardPadding = css`
padding: ${ CONFIG.cardPaddingXSmall };
`;
export const cardPaddings = {
none: css`
padding: 0;
`,
large: css`
padding: ${ CONFIG.cardPaddingLarge };
`,
medium: css`
padding: ${ CONFIG.cardPaddingMedium };
`,
small: css`
padding: ${ CONFIG.cardPaddingSmall };
`,
xSmall: xSmallCardPadding,
// The `extraSmall` size is not officially documented, but the following styles
// are kept for legacy reasons to support older values of the `size` prop.
extraSmall: xSmallCardPadding,
};
const xSmallCardPadding = css`
padding: ${ space( 2 ) };
`;
export const cardPaddings = {
none: css`
padding: 0;
`,
large: css`
padding: ${ space( 6 ) } ${ space( 8 ) };
`,
medium: css`
padding: ${ space( 4 ) } ${ space( 6 ) };
`,
small: css`
padding: ${ space( 4 ) } ${ space( 4 ) };
`,
xSmall: xSmallCardPadding,
// The `extraSmall` size is not officially documented, but the following styles
// are kept for legacy reasons to support older values of the `size` prop.
extraSmall: xSmallCardPadding,
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 5c873be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Card: support more padding options

4 participants