[CDX-436] add product swatch for product card by niizom · Pull Request #45 · Constructor-io/constructorio-ui-components

niizom · 2026-04-15T15:59:22Z

Pull Request Checklist

Before you submit a pull request, please make sure you have to following:

I have added or updated TypeScript types for my changes, ensuring they are compatible with the existing codebase.
I have added JSDoc comments to my TypeScript definitions for improved documentation.
I have added tests that prove my fix is effective or that my feature works.
I have added any necessary documentation (if appropriate).
I have made sure my PR is up-to-date with the main branch.

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no API changes)
Documentation content changes
TypeScript type definitions update
Other... Please describe:

Copilot

Pull request overview

Adds product swatches to the ProductCard so users can switch variants (name/image/price/variationId) directly from the card.

Changes:

Introduces swatch types and a useProductSwatch hook to manage swatch list + selection state.
Updates ProductCard to render a new SwatchSection and to derive a “display product” from the selected swatch.
Adds Storybook stories and unit tests covering swatch rendering and interaction.

Reviewed changes

Copilot reviewed 9 out of 9 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 between color vs image swatch rendering.
`src/utils/productDisplay.ts`	Adds `getDisplayProduct` helper to compute the displayed variant data from the selected swatch.
`src/utils/index.ts`	Re-exports `productDisplay` utilities for internal consumption.
`src/types/productCardTypes.ts`	Adds swatch-related types and new `ProductCard` props/overrides for swatch behavior.
`src/hooks/useProductSwatch.ts`	New hook to build the swatch list and track the selected swatch.
`src/components/product-card.tsx`	Renders swatches, wires callbacks, and updates displayed product + tracking attributes.
`src/stories/components/ProductCard/ProductCard.stories.tsx`	Adds Storybook stories showcasing color/image swatches and callbacks.
`spec/hooks/useProductSwatch.test.ts`	Adds unit tests for swatch list building and selection behavior.
`spec/components/product-card/product-card.test.tsx`	Adds integration tests for swatch UI behavior and tracking attributes.

Comments suppressed due to low confidence (1)

src/components/product-card.tsx:496

handleProductClick dispatches the click event and calls onProductClick with product from props, while the card’s tracking data attributes and the rendered content are based on displayProduct (selected swatch). After selecting a swatch, this makes the click event payload/callback inconsistent with what the user is seeing and with the updated data-cnstrc-item-variation-id. Use the displayed/selected product (e.g. displayProduct or renderProps.product from context) for the click event detail and onProductClick.

  const handleProductClick = useCallback(
    (e: React.MouseEvent) => {
      const target = e.target as HTMLElement;

      // Do not fire if a conversion button (AddToCart / Wishlist) or swatch is clicked
      if (
        target.closest('[data-cnstrc-btn]') ||
        target.closest('.cio-product-card-swatch-section')
      ) {
        return;
      }

      dispatchCioEvent(CIO_EVENTS.productCard.click, { product }, e.currentTarget);
      onProductClick?.(product);
    },

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-15T16:05:42Z

+export function useProductSwatch(product: Product): ProductSwatchObject {
+  const swatchList = useMemo(() => buildSwatchList(product), [product]);
+
+  const [selectedSwatch, setSelectedSwatch] = useState<SwatchItem | undefined>(() =>
+    swatchList.find((swatch) => swatch.variationId === product.variationId),
+  );


selectedSwatch is initialized from swatchList once, but it won’t update if the product prop changes (including product.variationId or product.swatchList). This can leave stale selection when the hook is reused for a different product or when swatches load asynchronously. Sync selectedSwatch when product.variationId/swatchList changes (e.g. via an effect that resets selection to the matching swatch, or clears it when none match).

Copilot · 2026-04-15T16:05:42Z

+  price?: string | number;
+  salePrice?: string | number;
+  swatchPreview: string;
+  variationId?: string;


SwatchItem.variationId is optional in the type, but the swatch UI relies on it for React keys, data-testid, data-cnstrc-item-variation-id, selection comparison, and (as a fallback) aria-label. If a consumer omits variationId, this will produce duplicate/unstable keys and can create unlabeled buttons (accessibility issue). Either make variationId required for SwatchItem (recommended) or update the UI to generate a stable fallback key/id and always provide a non-empty accessible label.

Suggested change

variationId?: string;

variationId: string;

this is a good call. there should always be a variation-id tied to a particular swatch imo. wdyt?

Copilot · 2026-04-15T16:05:43Z

@@ -0,0 +1,13 @@
+import { Product, SwatchItem } from '@/types/productCardTypes';


getDisplayProduct only uses Product/SwatchItem as TypeScript types, but they’re imported as values. The codebase consistently uses import type for type-only imports (e.g. src/utils/events.ts:1), and using value imports here can introduce unnecessary runtime dependencies/circular-import risk when re-exported via src/utils/index.ts. Switch this to a type-only import.

Suggested change

import { Product, SwatchItem } from '@/types/productCardTypes';

import type { Product, SwatchItem } from '@/types/productCardTypes';

constructor-claude-bedrock

Code Review

This PR adds a product swatch feature to the ProductCard component, including a new useProductSwatch hook, SwatchSection compound component, type definitions, utility helpers, stories, and comprehensive tests — overall a solid, well-structured implementation.

Inline comments: 8 discussions added

Overall Assessment: ⚠️ Needs Work

constructor-claude-bedrock · 2026-05-15T17:04:33Z

+export function useProductSwatch(product: Product, maxSwatches?: number): ProductSwatchObject {
+  const swatchList = useMemo(() => buildSwatchList(product), [product]);
+
+  const [selectedSwatch, setSelectedSwatch] = useState<SwatchItem | undefined>(() =>


Critical Issue: The useState initializer runs only once on mount and captures the initial swatchList value. If product (and therefore swatchList) changes after mount — e.g. the parent passes a new product — selectedSwatch will never be re-initialised to match the new product.variationId. This creates a stale-state bug where the selected swatch remains from the previous product.

Consider a useEffect (or deriving the selected swatch from state via a controlled approach) that resets selectedSwatch whenever product.variationId or swatchList changes:

useEffect(() => { setSelectedSwatch(swatchList.find((s) => s.variationId === product.variationId)); }, [product.variationId, swatchList]);

constructor-claude-bedrock · 2026-05-15T17:04:33Z

  return twMerge(clsx(inputs));
 }
+
+export function isHexColor(value?: string): boolean {


Important Issue: The current implementation only handles 6-digit hex colours (#RRGGBB, length === 7). It will return false for valid 3-digit shorthands (#RGB, length === 4) and 8-digit hex-alpha values (#RRGGBBAA, length === 9) that are valid CSS. While this may be an intentional constraint, it should be documented with a comment to avoid future confusion, and the isHexColor function should reject hex strings like #GGG (non-hex digits after # are rejected by Number(0x...), but #0x1 would incorrectly pass — e.g. Number('0x0x1') is NaN so it's fine, but the use of Number() rather than a regex is fragile). A clearer approach:

export function isHexColor(value?: string): boolean { return typeof value === 'string' && /^#[0-9A-Fa-f]{6}$/.test(value); }

constructor-claude-bedrock · 2026-05-15T17:04:33Z

+  price?: string | number;
+  salePrice?: string | number;
+  swatchPreview: string;
+  variationId?: string;


Important Issue: SwatchItem.variationId is typed as optional (variationId?: string). However, the entire swatch feature depends on variationId for key, data-testid, data-cnstrc-item-variation-id, aria-pressed selection comparison, and selectSwatch identity. Without it, swatches render with key={undefined} (React warning), data-testid='cio-swatch-undefined', and selection state breaks. This field should be required (variationId: string) — or the component must gracefully handle its absence and tests should cover that edge case.

constructor-claude-bedrock · 2026-05-15T17:04:33Z

+    [renderProps, selectSwatch],
+  );
+
+  const defaultSetUrl = useCallback((url: string) => {


Suggestion: defaultSetUrl uses window.location.assign, which will throw in SSR / Node environments. The rest of the codebase already guards against SSR (see getPreferredColorScheme in styleHelpers.ts). Add a guard here:

const defaultSetUrl = useCallback((url: string) => { if (typeof window !== 'undefined') { window.location.assign(url); } }, []);

constructor-claude-bedrock · 2026-05-15T17:04:34Z

+    return labelProp ?? 'View more >';
+  }, [props.showMoreSwatchesLabel, renderProps.showMoreSwatchesLabel, hiddenSwatches.length]);
+
+  if (!swatchList.length) return null;


Suggestion: The early-return guard (if (!swatchList.length) return null) is placed after the useMemo for showMoreLabel. This is fine for correctness, but the early return means the memoised label computation runs even when there are no swatches. Moving the guard before the useMemo (or before all hooks) is not possible due to React's rules of hooks. The existing placement is correct, but the showMoreLabel memo could be conditional on swatchList.length > 0 to avoid unnecessary work when the list is empty.

Mudaafi

First pass review: great job porting over the PLP implementation into shared components. It took a while to figure out what we're doing w/o a PR description :p but I like the choices you've made here. I left some comments around naming and structuring before I review the new docs so ping me when it's ready for a second review

Mudaafi · 2026-06-11T22:02:11Z

@@ -0,0 +1,13 @@
+import { Product, SwatchItem } from '@/types/productCardTypes';
+
+export function getDisplayProduct(product: Product, selectedSwatch?: SwatchItem): Product {


Suggested change

export function getDisplayProduct(product: Product, selectedSwatch?: SwatchItem): Product {

export function getProductWithVariationRollup(product: Product, selectedSwatch?: SwatchItem): Product {

or some equivalent

Mudaafi · 2026-06-11T22:13:12Z

    description?: ComponentOverrideProps<ProductCardProps>;
    rating?: ComponentOverrideProps<ProductCardProps>;
    price?: ComponentOverrideProps<ProductCardProps>;
+    swatch?: ComponentOverrideProps<ProductCardProps>;


Suggested change

swatch?: ComponentOverrideProps<ProductCardProps>;

swatches or productSwatch?: ComponentOverrideProps<ProductCardProps>;

Mudaafi · 2026-06-11T22:35:02Z

+        ...getProductCardDataAttributes(displayProduct),
+      },
      componentOverrides,
+      swatch,


shouldn't this be inside the renderProps object? It'll be hard for a custom swatch component to trigger a re-render without access to the swatch.selectSwatch function

Mudaafi · 2026-06-11T23:14:17Z

+function buildSwatchList(product: Product): SwatchItem[] {
+  if (!product.swatchList?.length) return [];
+
+  return product.swatchList.reduce<SwatchItem[]>((list, item) => {
+    if (item.swatchPreview) {
+      list.push({
+        name: item.name || product.name,
+        url: item.url,
+        imageUrl: item.imageUrl || product.imageUrl,
+        price: item.price ?? product.price,
+        salePrice: item.salePrice ?? product.salePrice,
+        swatchPreview: item.swatchPreview,
+        variationId: item.variationId,
+        rolloverImage: item.rolloverImage,
+      });
+    }
+    return list;
+  }, []);
+}


Correct me if I'm wrong but I don't think we need this since the parent component re-creates the rendered fields using utils.getDisplayProduct right? In that case, the swatchList can just be product.swatchList

Mudaafi · 2026-06-11T23:18:38Z

 }

 function ProductCard({ componentOverrides, children, className, ...props }: ProductCardProps) {
+  const swatch = useProductSwatch(props.product, props.maxSwatches);


wdyt of expanding the individual fields and including it as part of renderProps? I'm cool either way, but it feels a little confusing that swatch contains methods too

Mudaafi · 2026-06-11T23:23:55Z

+  price?: string | number;
+  salePrice?: string | number;
+  swatchPreview: string;
+  variationId?: string;


this is a good call. there should always be a variation-id tied to a particular swatch imo. wdyt?

Mudaafi · 2026-06-11T23:26:57Z

+  onSwatchClick?: (e: React.MouseEvent, swatch: SwatchItem) => void;
+  onShowMoreSwatches?: (
+    event: React.MouseEvent,
+    selectedSwatch: SwatchItem | undefined,
+    hiddenSwatches: SwatchItem[],
+    setUrl: (url: string) => void,
+  ) => void;
+  maxSwatches?: number;
+  showMoreSwatchesLabel?: string | ((hiddenCount: number) => string);


This is fine since SwatchSection is a sub-component of ProductCard and so they are tightly coupled to each other. Just calling this out because it's against the pattern of Compound Components which would prefer you override the nested components directly if you want to pass props.

Mudaafi · 2026-06-11T23:27:51Z

@@ -0,0 +1,13 @@
+import { Product, SwatchItem } from '@/types/productCardTypes';


Mudaafi · 2026-06-11T23:31:57Z

  });
+
+  describe('Swatch Section', () => {
+    const mockProductWithSwatches = {


Can you update the main mockProductData with swatch information instead? It should be part of the default mock imo to ensure it doesn't break other things

Mudaafi · 2026-06-11T23:36:02Z

+      expect(card.getAttribute('data-cnstrc-item-variation-id')).toBe('var-2');
+    });
+
+    test('component override for swatch section works', () => {


great test. Can we also include a separate test that ensures that most/all of the current swatch implementation can be created by whatever renderProps are passed to the overridden component?

niizom requested a review from a team as a code owner April 15, 2026 15:59

Copilot AI review requested due to automatic review settings April 15, 2026 15:59

Copilot started reviewing on behalf of niizom April 15, 2026 16:00 View session

This comment was marked as outdated.

Sign in to view

niizom requested review from HHHindawy, TarekAlQaddy and esezen April 15, 2026 16:05

Copilot AI reviewed Apr 15, 2026

View reviewed changes

[CDX-436] add product swatch for product card

11fa062

niizom force-pushed the cdx-436-add-product-swatch-for-shared-components-product-card branch from 4f4d960 to 11fa062 Compare May 5, 2026 13:08

This comment was marked as outdated.

Sign in to view

[CDX=436]: add view more button for product swatches

05f269a

constructor-claude-bedrock Bot reviewed May 15, 2026

View reviewed changes

Mudaafi reviewed Jun 11, 2026

View reviewed changes

		@@ -0,0 +1,13 @@
		import { Product, SwatchItem } from '@/types/productCardTypes';

	import { Product, SwatchItem } from '@/types/productCardTypes';
	import type { Product, SwatchItem } from '@/types/productCardTypes';

		@@ -0,0 +1,13 @@
		import { Product, SwatchItem } from '@/types/productCardTypes';

		export function getDisplayProduct(product: Product, selectedSwatch?: SwatchItem): Product {

	export function getDisplayProduct(product: Product, selectedSwatch?: SwatchItem): Product {
	export function getProductWithVariationRollup(product: Product, selectedSwatch?: SwatchItem): Product {

	swatch?: ComponentOverrideProps<ProductCardProps>;
	swatches or productSwatch?: ComponentOverrideProps<ProductCardProps>;

Uh oh!

Conversation

niizom commented Apr 15, 2026

Pull Request Checklist

PR Type

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

constructor-claude-bedrock Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

constructor-claude-bedrock Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

constructor-claude-bedrock Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

constructor-claude-bedrock Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

constructor-claude-bedrock Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

constructor-claude-bedrock Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mudaafi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!