Skip to content

refactor(card): rework Card component architecture and API#1359

Draft
misama-ct wants to merge 15 commits intomainfrom
proposal/rework-card-component
Draft

refactor(card): rework Card component architecture and API#1359
misama-ct wants to merge 15 commits intomainfrom
proposal/rework-card-component

Conversation

@misama-ct
Copy link
Copy Markdown
Collaborator

@misama-ct misama-ct commented Apr 13, 2026

Note

Card was one of the early components and we made a couple of questionable decisions. This PR addresses them. There are breaking changes, but fortunately no consumers yet.

What changed

Architecture: direct rendering replaces context registration

Card.Header and Card.Content previously returned null and used useEffect to register JSX into parent state via context callbacks. This pattern — unique among all Nimbus compound components — introduced re-render risk, stale closures, and a double render cycle.

Now Header, Body, and Footer are simple passthrough components that render their slot directly, matching Dialog.Body, Dialog.Footer, and every other Nimbus compound component.

Naming: Content → Body, Footer added

  • Card.Content renamed to Card.Body (aligns with Dialog.Body, Drawer.Body convention)
  • Card.Footer added (matches Dialog, DefaultPage, and every major UI library)

Props: four visual props → standard variant + size

cardPadding, borderStyle, elevation, and backgroundStyle are replaced by the standard Nimbus variant and size props.

size (sm | md | lg, default: md) — sets --card-spacing CSS variable:

Size --card-spacing
sm spacing.300
md (default) spacing.400
lg spacing.600

variant (outlined | elevated | filled | plain, default: outlined):

Variant Border Shadow Background
outlined (default) solid-25, colorPalette.6 none bg
elevated none shadow 1 bg
filled none none colorPalette.2
plain none none bg

Spacing: CSS-only padding distribution via --card-spacing

A unified --card-spacing CSS variable controls all slot padding. Each slot gets p: var(--card-spacing). When two card slots are directly adjacent, the later slot suppresses its top padding via adjacent sibling selectors. When a non-slot element (e.g. Separator) sits between slots, both retain full padding for visually balanced spacing.

Accessibility: slot-based ARIA wiring

Card.Root uses React Aria's useSlotId + Provider pattern to automatically wire aria-labelledby and aria-describedby when <Heading slot="title"> or <Text slot="description"> are present. Cards with slots get role="article"; cards without remain plain <div>s.

Layout: inline-flexflex

Cards are now block-level containers that fill available width by default.


Consumer API

<Card.Root variant="elevated" size="md">
  <Card.Header>Product details</Card.Header>
  <Card.Body>Description and metadata</Card.Body>
  <Card.Footer>Action buttons</Card.Footer>
</Card.Root>

Files

  • openspec/changes/rework-card-component/ — proposal, spec delta, and task list
  • packages/nimbus/src/components/card/ — full implementation, stories, docs spec, MDX docs

Test plan

  • pnpm --filter @commercetools/nimbus typecheck passes
  • pnpm lint passes
  • pnpm test:dev packages/nimbus/src/components/card/card.stories.tsx — all stories pass
  • pnpm test:dev packages/nimbus/src/components/card/card.docs.spec.tsx — all doc specs pass
  • Visual review in Storybook: all variant/size combinations, part combinations, separator spacing, slot-based accessibility

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nimbus-documentation Ready Ready Preview, Comment Apr 15, 2026 8:23am
nimbus-storybook Ready Ready Preview, Comment Apr 15, 2026 8:23am

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 13, 2026

⚠️ No Changeset found

Latest commit: 47c8ad1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@misama-ct misama-ct self-assigned this Apr 13, 2026
@misama-ct misama-ct requested a review from ByronDWall April 13, 2026 12:56
@misama-ct
Copy link
Copy Markdown
Collaborator Author

@claude do a thorough review of this branches changes

@misama-ct misama-ct changed the title proposal(card): rework Card component architecture and API refactor(card): rework Card component architecture and API Apr 14, 2026
Proposes removing the context-registration anti-pattern, renaming
Content to Body, adding Footer, and aligning variant props with
Nimbus conventions (variant + size).
…d standard variant/size API

Remove the context-based child registration pattern (useState + useEffect)
that caused re-render risks, replacing it with direct rendering matching
all other Nimbus compound components. Rename Card.Content to Card.Body for
naming consistency, add Card.Footer sub-component, and replace the four
visual props (cardPadding, borderStyle, elevation, backgroundStyle) with
standard Nimbus variant/size props. Padding is now CSS-only via
:first-child/:last-child pseudo-selectors.
Rewrite stories to use Card.Body, Card.Footer, variant, and size props
with play functions for all combinations. Update docs spec with footer and
variant/size test cases. Update MDX documentation (dev, a11y, guidelines)
to replace all old prop references.
…adding

Extract repeated slot padding rules into a shared --card-padding CSS
variable set on root per size, eliminating 3x duplication across
header/body/footer slots.
The variable drives spacing for both padding and gap, so the name
should not imply padding only.
…ers into HeaderBodyFooter story

Move gap to base (derives from --card-spacing like slot padding), so size
variants only set the single CSS variable. Merge the WithDividers story
into HeaderBodyFooter as a side-by-side comparison across all sizes.
…alette.6 for border

Merge HeaderBodyFooter, BodyOnly, HeaderAndBody, and WithDividers into a
single PartCombinations story showing all realistic part permutations
across sizes with and without dividers. Change outlined border from
colorPalette.3 to colorPalette.6 to match the Separator color.
spacing.200 (8px) felt too condensed for the sm size. Bump to
spacing.300 (12px) for a more comfortable small variant. Final scale:
sm=12px, md=16px, lg=24px — matching Mantine's card sizing.
Slots now own their spacing via full padding instead of root gap.
Adjacent sibling selectors collapse doubled padding when slots
sit next to each other; separators break adjacency so both sides
keep full padding naturally.
Card.Root now uses React Aria's useSlotId + Provider pattern to
automatically wire aria-labelledby and aria-describedby when consumers
place <Heading slot="title"> or <Text slot="description"> inside the
card. The card conditionally gains role="article" only when at least
one slot is detected — cards without slots remain plain divs.

Follows the Adobe React Spectrum Card spec (adobe/react-spectrum#2080).
Uses the slots map format with DEFAULT_SLOT for strict slot matching,
ensuring plain <Heading> and <Text> components are unaffected.
…mentation

- Update spec and proposal to reflect actual values: colorPalette.6 for
  outlined border, spacing.300 for sm size, unified --card-spacing model
  instead of separate gap tokens
- Document adjacent sibling selector strategy (not :first-child/:last-child)
  in spec and proposal, with explanation of intentional Separator spacing
- Remove unnecessary `as any` cast from Provider values in card.root.tsx
- Add comments to recipe explaining adjacent-sibling padding collapsing
- Mark tasks 13–18 as complete in tasks.md
Update frontpage and category-overview to use the reworked Card API:
- Card.Content → Card.Body
- borderStyle prop → variant prop
- cardPadding prop → size prop
- Make frontpage nav cards responsive (1 col mobile, 2 col desktop)
Replace Card.Content with Card.Body and replace borderStyle/elevation
props with the new variant prop after the Card component rework.
@misama-ct
Copy link
Copy Markdown
Collaborator Author

@claude please review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 20, 2026

Claude finished @misama-ct's task in 3m 59s —— View job


Review: refactor(card): rework Card component architecture and API

  • Gather context and understand the request
  • Read core implementation files
  • Read slot components
  • Read tests and stories
  • Read docs files
  • Review app-level changes
  • Analyze and post review

Summary

This is a well-motivated refactor. The old context-registration pattern (useEffect → parent state via callbacks) was the worst possible approach for a compound component — it guaranteed a double-render cycle, stale-closure risk, and incompatibility with Strict Mode's double-invoke behavior. The new direct-rendering approach is idiomatic and correct. The API surface (variant/size) aligns with the rest of Nimbus, and the CSS-variable spacing model is clean. Overall the architecture is solid. A few issues need attention before merge.


Must fix before merge

1. Missing changeset

This PR contains breaking changes (Card.ContentCard.Body, four props replaced with two). The changeset bot already flagged this. Add a changeset:

pnpm changeset

Choose @commercetools/nimbus: minor (not patch — this is a breaking API change).


Should address

2. card.figma.tsx only covers 2 of 4 variants [packages/nimbus/src/components/card/card.figma.tsx:39]

The Code Connect file maps a binary "Elevated" Figma enum to elevated / outlined. The new filled and plain variants aren't represented. If the Figma component hasn't been updated yet, track this as follow-up work so the spec and implementation don't diverge silently.

3. Adjacent sibling selectors are tightly coupled to generated class names [packages/nimbus/src/components/card/card.recipe.ts:32-39]

".nimbus-card__header + &": { pt: 0 },
".nimbus-card__body + &": { pt: 0 },

These selectors depend on Chakra generating nimbus-card__header / nimbus-card__body from className: "nimbus-card" and the slot names. If either the className base or the slot names change, these selectors silently stop working. The comment explains the behavior well — add a note explaining the naming convention so the coupling is explicit:

// Class names are Chakra-generated: `${className}__${slotName}` = "nimbus-card__header"

4. Doc spec doesn't assert positive ARIA wiring [packages/nimbus/src/components/card/card.docs.spec.tsx:86-106]

The negative case (has no role or ARIA attributes without slots) correctly asserts the absence of attributes. But the positive case only checks that text renders — it doesn't verify role="article", aria-labelledby, or aria-describedby are actually set. Since this is consumer-facing documentation, it would strengthen the example:

it("renders accessible card with title and description slots", () => {
  // ...render...
  const card = screen.getByTestId("card-slot");
  expect(card).toHaveAttribute("role", "article");
  expect(card).toHaveAttribute("aria-labelledby");
  expect(card).toHaveAttribute("aria-describedby");
});

Note: useSlotId relies on a state update, so this test may need waitFor in JSDOM. If it's flaky, the story tests are already the authoritative coverage — just add a note explaining why the doc spec can't verify the attribute.

5. Interactive card docs show a semantic conflict [packages/nimbus/src/components/card/card.dev.mdx:151-173]

"Option 2" wraps Card.Root in a Box role="button". If that consumer also uses <Heading slot="title">, the card gets role="article" — an article nested inside a button is invalid ARIA (article is a landmark; landmark roles must not be descendants of interactive roles). The example should either:

  • add a note: "Don't use slot-based labeling when the card is inside a button wrapper — use aria-label on the outer element instead", or
  • replace the role="button" approach entirely with a real <button> (preferred).

Fix this →


Minor observations

6. Missing story for description-only slot case

TitleSlotOnly is tested. The a11y table in card.a11y.mdx documents Text slot="description" without a heading slot as a distinct supported case, but there's no story that verifies it. Not blocking, but the table makes a claim the test suite doesn't back.

7. alignItems: "flex-start" on root [packages/nimbus/src/components/card/card.recipe.ts:17]

Slot children are covered by their width: "100%". But free-form non-slot content placed directly in Card.Root will shrink-wrap instead of stretching. The WithoutCompound story renders a bare <div> inside the root — consumers might expect it to fill width. This is technically intentional given how the slots work, but it's worth a comment clarifying the behavior.

8. focusVisibleRing: "outside" in base styles [packages/nimbus/src/components/card/card.recipe.ts:19]

This style is unconditionally applied even to non-interactive cards. That's harmless (unfocusable elements never show the ring), but a brief comment would clarify this is in anticipation of tabIndex={0} usage rather than a mistake.


Architecture sign-off

  • Direct rendering replacing context registration: correct and consistent with every other Nimbus compound component. ✓
  • useSlotId + Provider pattern for ARIA wiring: idiomatic React Aria, tested end-to-end in stories. ✓
  • --card-spacing CSS variable with adjacent sibling collapsing: clean, zero JS overhead. ✓
  • variant / size instead of four ad-hoc props: correct normalization. ✓
  • @react-aria/utils added as explicit dependency (was previously transitive): good hygiene. ✓
  • display: flex vs inline-flex: correct for block-level containers. ✓
  • App-level migrations (category-overview, frontpage): both correctly use Card.Body and compile cleanly. ✓

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants