[cdx-436]: add-product-swatches-for-product-card#52
Conversation
niizom
force-pushed
the
cdx-436-add-product-swatches-for-product-card
branch
from
4950df9 to
514431a
Compare
niizom
changed the title
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR adds a well-structured product swatch feature to ProductCard, including a useProductSwatch hook, a SwatchSection compound component, type definitions, Storybook stories, and comprehensive tests.
Inline comments: 7 discussions added
Overall Assessment:
| return twMerge(clsx(inputs)); | ||
| } | ||
|
|
||
| export function isHexColor(value?: string): boolean { |
There was a problem hiding this comment.
Important Issue: isHexColor only handles 6-digit hex colors (e.g. #e04062), but CSS and common usage also includes 3-digit hex (#f00) and 8-digit hex with alpha (#e04062ff). A swatch configured with a 3-digit color like #f00 will silently fall through to the url(...) background path, producing a broken style. Either expand the regex to handle all valid hex lengths or document the constraint explicitly.
Suggested implementation:
export function isHexColor(value?: string): boolean {
return typeof value === 'string' && /^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6}|[0-9a-fA-F]{8})$/.test(value);
}| { swatchList = [], variationId }: Product, | ||
| maxSwatches?: number, | ||
| ): ProductSwatchObject { | ||
| const [selectedSwatch, setSelectedSwatch] = useState<SwatchItem | undefined>(() => |
There was a problem hiding this comment.
Important Issue: The initial selectedSwatch is set once from product.variationId via the lazy initializer, but it is never re-synced if the parent passes a new product prop (e.g., when navigating between products or when the same ProductCard is reused for a different product). The stale swatch from the previous product would remain selected.
Add a useEffect (or restructure with useMemo + a key reset strategy on the parent) to reset selectedSwatch when variationId or swatchList identity changes:
useEffect(() => {
setSelectedSwatch(swatchList.find((s) => s.variationId === variationId));
}, [variationId, swatchList]);| } | ||
|
|
||
| const visible = swatchList.slice(0, maxSwatches); | ||
| const hidden = swatchList.slice(maxSwatches); |
There was a problem hiding this comment.
Suggestion: The hidden variable is computed only to call .length > 0 on it, which allocates a throwaway array. Use a direct comparison instead:
return { visibleSwatches: visible, hasMoreSwatches: swatchList.length > maxSwatches };| props.showViewMoreSwatches ? props.maxSwatches : undefined, | ||
| ); | ||
| const displayProduct = useMemo(() => { | ||
| const filtered = Object.fromEntries( |
There was a problem hiding this comment.
Important Issue: Object.fromEntries(Object.entries(...).filter(...)) drops only undefined values but leaves null, 0, false, and '' in, which is probably intentional. However, it also inadvertently copies the variationId from the swatch into displayProduct. When a swatch is selected, displayProduct.variationId becomes the swatch's variationId, which is then passed to getProductCardDataAttributes and rendered as data-cnstrc-item-variation-id on the card root. This may be intentional for analytics, but it also means swatch deselection resets the attribute back to the original product's variationId (since selectedSwatch returns undefined). Verify this is the desired analytics behavior and, if so, add a comment explaining it. If not, explicitly exclude variationId from the merge:
const { variationId: _unused, swatchList: _swatchList, ...swatchData } = swatch.selectedSwatch || {};
return { ...props.product, ...Object.fromEntries(Object.entries(swatchData).filter(([, v]) => v !== undefined)) };| componentOverrides, | ||
| }), | ||
| [props, componentOverrides], | ||
| [props, displayProduct, swatch, componentOverrides], |
There was a problem hiding this comment.
Important Issue: swatch (the full object returned by useProductSwatch) is included as a useMemo dependency. Because useProductSwatch returns a new object reference on every render (object literal { selectedSwatch, visibleSwatches, ... }), this defeats the memoization of contextValue — it will recompute on every render even when nothing changed. Destructure the stable primitives instead:
[props, displayProduct, swatch.selectedSwatch, swatch.visibleSwatches, swatch.hasMoreSwatches, swatch.onSwatchClick, swatch.onViewMoreSwatchesClick, componentOverrides]Alternatively, return a useMemo-stabilized object from useProductSwatch itself.
| onViewMoreSwatchesClick?.(e, selected); | ||
| }, | ||
| [product, onProductClick], | ||
| [expandInline, swatch.onViewMoreSwatchesClick, onViewMoreSwatchesClick], |
There was a problem hiding this comment.
Suggestion: Referencing swatch.onViewMoreSwatchesClick directly from the destructured hook return breaks the eslint react-hooks/exhaustive-deps rule expectation (accessing a property of a non-stable object in deps). Extract it before the callback:
const { onViewMoreSwatchesClick: expandSwatchesInline } = swatch;
const handleViewMoreSwatchesClick = useCallback(
(e, selected) => {
if (expandInline) expandSwatchesInline();
onViewMoreSwatchesClick?.(e, selected);
},
[expandInline, expandSwatchesInline, onViewMoreSwatchesClick],
);| 'cio:outline-3 cio:outline-offset-[4px] cio:outline-current cio:opacity-60', | ||
| )} | ||
| style={{ background: bgValue }} | ||
| onClick={(e) => onSwatchClick?.(e, swatchItem)} |
There was a problem hiding this comment.
Suggestion: An inline arrow function is created for every swatch button on every render. For a product with many swatches this creates noticeable GC pressure. Since onSwatchClick itself is already memoized in the hook, wrap the per-item handler using a data attribute or pass the item ID and look it up, or lift the handler to accept an item reference directly. At minimum, consider a shared handler on the container that reads data-cnstrc-item-variation-id from the event target to avoid per-item closures.
There was a problem hiding this comment.
Pull request overview
Adds product swatches (color/variant selectors) to the ProductCard component, including optional “View more” truncation/expansion behavior, with Storybook docs/examples and test coverage.
Changes:
- Introduces swatch-related types and a new
useProductSwatchhook to manage selection + truncation/expansion state. - Updates
ProductCardto render aSwatchSection, apply selected-swatch overrides to displayed product data, and emit events/callbacks using the displayed variant. - Adds Storybook stories/docs and unit/component tests for swatch behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/styleHelpers.ts | Adds isHexColor helper used to decide hex-vs-image swatch previews. |
| src/types/productCardTypes.ts | Adds swatch types and new ProductCard props/override hooks for swatches. |
| src/hooks/useProductSwatch.ts | New hook for swatch selection and “view more” expansion logic. |
| src/components/product-card.tsx | Renders swatches, computes displayProduct, and wires “view more” behavior + overrides. |
| src/stories/components/ProductCard/UsagePatterns.mdx | Documents new swatch usage patterns in Storybook. |
| src/stories/components/ProductCard/ProductCard.stories.tsx | Adds swatch-focused stories (basic, image swatches, view-more). |
| src/stories/components/ProductCard/Code Examples - Swatches.mdx | New swatch code example documentation page. |
| src/stories/components/ProductCard/Code Examples - Compound Components.mdx | Documents new ProductCard.SwatchSection compound component. |
| spec/hooks/useProductSwatch.test.ts | Adds unit tests for hook selection/truncation/expansion. |
| spec/components/product-card/product-card.test.tsx | Adds component tests validating UI, data overrides, and view-more behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { useMemo, useState, useCallback } from 'react'; | ||
| import type { Product, SwatchItem, ProductSwatchObject } from '@/types/productCardTypes'; | ||
|
|
||
| export function useProductSwatch( | ||
| { swatchList = [], variationId }: Product, | ||
| maxSwatches?: number, | ||
| ): ProductSwatchObject { | ||
| const [selectedSwatch, setSelectedSwatch] = useState<SwatchItem | undefined>(() => | ||
| swatchList.find((swatch) => swatch.variationId === variationId), | ||
| ); | ||
| const [isExpanded, setIsExpanded] = useState(false); | ||
|
|
||
| const onSwatchClick = useCallback((swatch: SwatchItem) => { | ||
| setSelectedSwatch((selectedSwatch) => { | ||
| if (selectedSwatch?.variationId === swatch.variationId) return undefined; | ||
| else return swatch; | ||
| }); | ||
| }, []); | ||
|
|
||
| const onViewMoreSwatchesClick = useCallback(() => { | ||
| setIsExpanded(true); | ||
| }, []); | ||
|
|
| description?: ComponentOverrideProps<ProductCardProps>; | ||
| rating?: ComponentOverrideProps<ProductCardProps>; | ||
| price?: ComponentOverrideProps<ProductCardProps>; | ||
| swatches?: ComponentOverrideProps<ProductCardProps>; |
| className?: string; | ||
| } | ||
|
|
||
| export interface SwatchSectionProps extends IncludeRenderProps<ProductCardProps> { |
Pull Request Checklist
Before you submit a pull request, please make sure you have to following:
PR Type
What kind of change does this PR introduce?