Repository navigation
Add composable catalog and product card parts - #1506
wcole1-godaddy merged 7 commits into
Conversation
Catalog is now composed from CatalogRoot with CatalogHeader, CatalogSummary, CatalogProducts and CatalogPagination, and ProductCard from ProductCardRoot with image, title, description, price and action parts. CatalogProducts renders its children as the card for every product, so card parts take no product, SKU or price props and samples share the same card. Applies the lessons from the product details review: host classes go on a commerce-part wrapper the CSS reset skips, catalog cards are direct grid children so placement classes work, every part uses its root's prefix, and the README layout is checked pixel-for-pixel against the packaged component. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cards stretch to their grid row and line up their price rows. Drops self-start, which kept the uneven heights from 0.2.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The purchase area cancelled its own gaps to match 0.2.0, so options, messages and quantity touched. Use a flex gap, with the status and stock messages 8px under the options. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: a3fcecc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ProductCardImage always hid its link from the keyboard and screen readers, assuming ProductCardTitle supplied one. A card without a title had no accessible product link. The title now tells the root it's rendered, and the image link is hidden only then. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The native arrow sat against the border and ignored padding. Draw a chevron 12px in from the edge. Also note in the README that host classes sharing a name with a package utility lose. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Say which Catalog the README layout writes out, note the smaller checking message in the spacing changeset, and export ProductCardProps alongside the catalog prop types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
wcole1-godaddy
left a comment
wcole1-godaddy
left a comment
There was a problem hiding this comment.
Approving since this is experimental, but please follow up on the first two inline comments soon. Both are regressions from the current behavior: host layout classes get overridden, and the image link is focusable before hydration. The other inline comments are small fixes worth folding in if you're touching the code anyway.
Non-blocking cleanup:
product-purchase.tsx:26: the localconst messageshadows themessage()formatter imported from./apiin the same file. Something likebelowOptionswould avoid confusion.product-card.tsx:110:ProductCardRootadds a second, state-based way to count mounted parts, alongside the ref-Mapregisterinproduct-details-context.tsx. It also copies thelabel ?? name ?? 'Product'fallback. A shared hook and agetProductName()helper would keep them in sync.ProductCardInternalcould be a discriminated union ({ source: 'live'; href: string } | { source: 'sample'; href: null }). That would remove thehref ?? ''anditem.source === 'live' && hrefguards.- The
CatalogContextand card context values are rebuilt on every render, so every card re-renders on any refetch.useMemowould avoid that.
| if (attributes.length === 0 && skuOptions.length === 0) return null; | ||
| return ( | ||
| <div className={partClass('space-y-6', className)}> | ||
| <div className={partClass('flex flex-col gap-6', className)}> |
There was a problem hiding this comment.
Switching from space-y-6 to flex flex-col gap-6 on the element that also gets the host className means the package's unlayered .flex and .gap-6 beat a Tailwind v4 host's layered utilities. For example, <ProductDetailsOptions className="grid grid-cols-2 gap-4" /> still renders as a single column with 24px gaps. The same applies to ProductDetailsPurchase (line 28) and CatalogPagination (flex justify-between, catalog.tsx:209). That contradicts the README's statement that className controls layout. The conflict test only checks margin, padding, border, color and font, so it doesn't catch this. Could the default layout go on an inner wrapper, or be skipped when the host passes a className?
There was a problem hiding this comment.
Fixed in a3fcecc. ProductDetailsOptions and ProductDetailsPurchase are back to space-y-6, which styles the children, so host display, gap and grid classes on the part apply. CatalogPagination's flex layout moved to an inner element. The conflict test now also checks display, gap, flex, grid columns and width, and that caught the same problem in ProductDetailsPrice (from #1502), which got the inner-element fix too. In a host grid, each control still keeps its 24px bottom margin, as on main; the README now says so. The test app has a new check for your example (grid grid-cols-2 on ProductDetailsOptions): it fails on the previous build and passes now. Default screenshots are pixel-identical.
| {item.source === 'live' && href ? ( | ||
| <Link | ||
| to={href} | ||
| tabIndex={hasTitleLink ? -1 : undefined} |
There was a problem hiding this comment.
hasTitleLink is only set by the useEffect on line 178, so the server HTML and the first client render expose this image link as a focusable, non-hidden duplicate of the title link. That's two tab stops and two same-named links per card until hydration, or permanently if JS fails. Every card also re-renders once after mount. The previous card hard-coded tabIndex={-1} and aria-hidden. Could this be decided during render, or hidden by default?
There was a problem hiding this comment.
Fixed in a3fcecc. The image link now starts hidden (tabIndex={-1}, aria-hidden), as in the previous card, so server HTML and the first render have one link per card. Titles register through a ref-based registry, and the root's effect runs after its children's, so only a card without a title link re-renders to expose the image link. The packaged card doesn't re-render. A new renderToString test covers this.
| */ | ||
| export function CatalogProducts({ className, children }: CatalogProductsProps): ReactElement { | ||
| const catalog = useCatalogInternal('CatalogProducts'); | ||
| const card: ReactNode = children ?? ( |
There was a problem hiding this comment.
children ?? only falls back for null and undefined. A conditional template like {useCustom && <ProductCardRoot>…</ProductCardRoot>} with useCustom=false renders an empty slot per product, while CatalogSummary still says "Showing N products". Consider checking children == null || children === false || children === '', or using Children.count.
There was a problem hiding this comment.
Fixed in a3fcecc. CatalogProducts now uses Children.toArray and ignores empty strings, so a false, null or empty template renders the packaged card. Added a test.
| </section> | ||
| </CatalogContext.Provider> | ||
| ) : ( | ||
| <CommerceStatus /> |
There was a problem hiding this comment.
The host className is dropped in this branch, so "Connecting to the store…" renders without the host's padding and width, and the layout jumps when the real <section> replaces it. Could this be wrapped in an element that applies the host classes?
There was a problem hiding this comment.
Fixed in a3fcecc. The connection status is wrapped in an element with the host classes. ProductDetailsRoot had the same problem, including its loading, error and not-found states, so those keep the host className now too. Added a test.
| if (status === 'ready') return null; | ||
| return ( | ||
| <div className={partClass(className)}> | ||
| {status === 'variant-error' ? ( |
There was a problem hiding this comment.
This nested ternary loses the exhaustiveness check the switch gave us: a new ProductDetailsStatus value would fall into the last branch silently. It also breaks the repo's "no nested ternaries" rule. Could we keep the switch?
There was a problem hiding this comment.
Fixed in a3fcecc. Restored the switch without a default case, so a new status leaves content unassigned and TypeScript reports it. I also removed the other nested ternaries this PR added (CatalogRoot, CatalogSummary).
…ge link - Price, Options, Purchase and Pagination keep their default layout off the element that gets the host className, so host display, gap and grid apply. Purchase is back to space-y-6; the -mt-4 messages collapse to the same 8px. - Roots keep the host className while connecting, loading or erroring. - The card's image link is hidden by default and exposed after mount only when no title link registered, so server HTML has one link per card. - A false or empty CatalogProducts template falls back to the packaged card. - Restore the purchase status switch; remove nested ternaries in new code. - Share the part registry between ProductDetailsRoot and ProductCardRoot, add getProductName, make the card context a discriminated union, and memoize catalog and card context values. - The conflict test now also checks display, gap, flex, grid columns and width. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. a3fcecc addresses all five inline comments (replies are on each). It also covers the non-blocking cleanup:
Package tests: 62 pass. Test app: smoke 15/15, parts 10/10, and all 34 screenshots are pixel-identical to the previous build. 🤖 Generated with Claude Code |
2 participants
Why
This is PR 2 of the compound components work started in #1502. It does for the catalog and product card what #1502 did for
ProductDetails. Layouts can be customized without forking or adding props, and the packaged components stay the default.Also fixed a couple minor visual issues with spacing.
AAB and other agents should keep using the packaged components,
<Catalog />and<ProductCard />. The parts are an opt-in option for when a layout genuinely needs more customization.The parts can't produce impossible states. Card parts take no product, SKU or price props. Inside
CatalogProducts,ProductCardRoottakes noproductprop either and throws if given one, so a card always shows the product it is rendered for. A direct add is still offered only for a product with one SKU and no options, and sample products only ever reach the sample cart.What
CatalogRoot(loads one cursor page, or the built-in samples whensampleProductsis set and the store is unbound),CatalogHeader,CatalogSummary,CatalogProductsandCatalogPagination.ProductCardRoot,ProductCardImage,ProductCardTitle,ProductCardDescription,ProductCardPrice,ProductCardAction, plus a read-onlyuseProductCard()hook.ProductCardImageis the card's accessible product link when there's noProductCardTitle; with a title, the title is the only link keyboard and screen reader users reach.CatalogProductsis the card, rendered once per product. Outside a catalog,ProductCardRootneeds aproductprop, for a featured product the host already has.CatalogandProductCardare now composed from these parts. The README has a "Custom catalog layouts" section with the packaged layout written out.Not repeating the #1502 fixes
classNameon acommerce-partwrapper the CSS reset skips. Catalog cards are now direct children of the grid, so host placement classes likecol-span-2work. The shipped-CSS conflict check from Add composable product details parts #1502 moved totest-css.tsand covers every catalog and card part, live and sample.Catalog*orProductCard*).<Catalog sampleProducts />, which it writes out.Visual changes
There's no backwards compatibility for these.
sm.ProductDetailspurchase area used to cancel its own gaps to match 0.2.0, so the options, messages and quantity touched. Options are now 24px apart, the status or stock message sits 8px under the options, and the quantity and button are 24px apart.<div>, catalog cards are direct grid children instead of each having its own surface wrapper, and the sample summary text is a<p>inside a<div>.Notes
<ProductCard />keeps its natural height. Even heights apply inside the catalog grid.Testing
Package checks: typecheck, lint, build and all 59 storefront tests pass, 7 of them new. They cover:
Test app (
commerce-component-test-app, installs thepnpm packtarball the way a host would):main. This PR adds checks for cursor pagination, a phone-width catalog, the sample catalog and cart, switching between a connected store and no store without a reload, and a standaloneProductCard.<Catalog />pixel-for-pixel on 4 screens: live, paged, sample and phone width. I ran the card-height check against the old build first, and it failed with a 24px difference.main: product pages, the not-found page, the cart drawer and the phone-width catalog are unchanged apart from the purchase-area spacing and the select chevron. A standaloneProductCardis pixel-identical tomain. I checked that each other difference comes from an intended change:main's screenshot reproduces this branch's screenshot exactly.