Repository navigation
Make storefront parts extensible without reimplementing their rules - #1511
sbolinger-godaddy merged 6 commits into
Conversation
- ProductDetailsRoot takes productId, urlState and loading/notFound/error replacements; it no longer requests /products/ without a product ID. - CatalogRoot and Catalog take pageSize; CatalogRoot takes urlState, and CatalogProducts takes empty. - Action and heading parts take replacement text, or a function of their state, while keeping their behavior. - CommerceProvider takes locale; useMoney and ProductImage are exported. - onEvent reports cart changes and failures; a throwing host callback no longer fails the cart operation. - Every part has a stable data-commerce-part styling hook. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 4ea2afa 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 |
Matches CartEmpty and the compound parts pattern: the empty state is its own part, placed anywhere in the layout, instead of a content prop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ules - CartItemRemove names its line with visually hidden text instead of aria-label, so a replaced label starts the accessible name (WCAG 2.5.3). The default name, "Remove <name>", is unchanged. - A sample card with a replaced action label drops its aria-label the same way. - README: one rule for replacing content (part text is children, loaded states are parts, pre-data root states are root props). - locale only formats; localized UI text will be a separate opt-in prop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
wesleycole
left a comment
wesleycole
left a comment
There was a problem hiding this comment.
Approved. This is a useful additive API: hosts can customize presentation and embed products without reimplementing the storefront's purchasing and checkout rules. Built the storefront package and verified all 103 existing tests and typecheck pass. Leaving three non-blocking comments covering empty-label fallback, image recovery, and asynchronous callback isolation; the first two were reproduced with targeted tests.
| ): ReactNode { | ||
| if (typeof label === 'function') { | ||
| const content: ReactNode = label(state); | ||
| return hasChildren(content) ? content : fallback; |
There was a problem hiding this comment.
Non-blocking: the empty-content fallback does not handle empty fragments. hasChildren() treats a fragment as content without inspecting its children, so a label such as () => <></> (or a fragment containing only null/false children) produces an unnamed add-to-cart button instead of the default label. I reproduced this with a targeted test. Could we recursively inspect fragments, handle blank strings, and add regression coverage for render-function results? The README's guarantee should also be narrowed: an arbitrary child component can render null, which this helper cannot generally detect.
| * A product image that fills its container, with a labeled placeholder when there is no URL or it fails to load. | ||
| * Use it in custom parts so they handle missing images like the package's own. | ||
| */ | ||
| export function ProductImage({ url, name, className = '' }: ProductImageProps): ReactElement { |
There was a problem hiding this comment.
Non-blocking: now that ProductImage is a supported public export, could we make it recover when url changes after an image failure? The existing failed boolean stays true for the lifetime of the component, so a custom SKU image that changes from a broken URL to a valid URL remains a placeholder and never requests the replacement image. I reproduced this by firing an error and rerendering with a new URL. Tracking the failed URL, or resetting failure on URL changes, plus a regression test would address it. This behavior predates this PR internally; the public export makes it worth addressing here.
| /** Runs a host callback so that its failure cannot fail the cart operation that triggered it. */ | ||
| function callHost(callback: () => void): void { | ||
| try { | ||
| callback(); |
There was a problem hiding this comment.
Non-blocking: callHost isolates synchronous throws, but not rejected promises. An integration such as onEvent={async (event) => { await sendAnalytics(event); }} is accepted by TypeScript and can produce an unhandled rejection if analytics fails. Could we observe the callback's returned promise and log rejections without awaiting it or delaying cart/checkout operations? Please add a rejecting-async-callback test for onEvent/onItemAdded. This would complete the callback-isolation behavior for common async integrations.
…callback rejections - hasChildren looks inside fragments and treats blank text as empty, so `() => <></>` keeps the package label. The README now says a label component that returns null cannot be detected. - ProductImage keys its failure to the URL that failed, so a new URL is requested. - callHost logs a rejected promise from onEvent or onItemAdded without awaiting it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All review comments addressed in 4ea2afa |
2 participants
Follow-up to #1509. Additive changes informed by a comparison with Shopify's Storefront Web Components, aimed at preventing bugs in host apps and avoiding cases where a host has to rewrite a part (and its rules) to make a small change.
Changes
Roots no longer depend on the route
ProductDetailsRoottakesproductId(falls back to the route's:productId) for quick views or products outside their route.urlState={false}keeps a root's selection out of the page URL, so two roots on one page no longer shareoption.*/sku./products/.CatalogRoot/CatalogtakepageSize(default 6).CatalogRoottakesurlState={false}for a second catalog on a page.CatalogEmptypart for a custom empty catalog, matchingCartEmpty. While one is mounted,CatalogProductsleaves out its default message.Replacement text that keeps behavior
CartButton,AddToCartButton,ProductDetailsAddToCart,ProductCardAction,CartCheckout,CartItemRemove,CartTitle,CartDescriptionandProductDetailsBackLinktake children, or a function of their state, e.g.({ busy }) => ….CartItemRemoveadds the product name as visually hidden text instead ofaria-label, so<CartItemRemove>Delete</CartItemRemove>is named "Delete Mug".ProductDetailsRoottakesloading,notFoundanderror({ message, retry }). These work like<Suspense fallback>: they replace the root's content before the product exists, so custom parts' hooks can still rely on a loaded product. The README states the rule: a part's text is its children, a state inside a loaded root is a part (CartEmpty,CatalogEmpty), and a state before the root has data is a root prop.Locale-aware prices
localeon the provider (defaulten-US; an invalid tag falls back with a dev warning). It only formats; localized UI text will be a separate opt-in prop, so settinglocalenever changes wording in a minor release.useMoney()for custom parts, an optional locale argument onmoney(), and an exportedProductImage.Events
onEventreportsitem-added,quantity-changed,item-removed,discount-applied,checkout-startedanderror. Sample events carryisSample: true.onItemAdded) is now logged. Before, it failed the cart operation and showed its message as a cart error.Stable styling hooks
data-commerce-parton every part and root, plusdata-commerce-controlanddata-commerce-attributewhere a part has several controls.README sections added: Replacing text, Styling hooks, Events, plus a quick-view example. Minor changeset included.
Testing
typecheck,lint,buildandtestpass. 106 tests, including 20 new ones insrc/extensibility.test.tsx.commerce-component-test-app, base (Add composable cart parts #1509) vs this branch: smoke, parts, walkthrough and boutique all pass. Screenshots are pixel-identical (57/57, zero threshold) and there were no console errors, package warnings or DOM audit failures.Screenshots
commerce-component-test-apprunning this branch's packed build (02cdeaf), with the mock store connected. Each one is pixel-identical to the #1509 baseline.