Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurable swatch limiting on product cards, including a new “View more” button that can either navigate to a PDP by default or invoke a new callback for custom behavior.
Changes:
- Introduces
swatchConfigs(max visible swatches + customizable “View more” label) and wires it throughCioPlp→CioPlpGrid→ProductCard→useProductInfo/useProductSwatch. - Adds “View more” button rendering and a new
onShowMoreSwatchescallback with a default navigation behavior. - Updates Storybook docs/examples and adds hook/component tests for the new behavior.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds SwatchConfigs, new callback type, and expands swatch hook return shape. |
| src/stories/hooks/UseProductSwatch/UseProductSwatchExample.tsx | Updates story example to pass item into ProductSwatch. |
| src/stories/hooks/UseProductSwatch/Props.mdx | Documents new hook config arg and additional return fields. |
| src/stories/components/ProductSwatch/RenderProps.md | Documents expanded render-props contract (now includes item + swatch partition fields). |
| src/stories/components/ProductSwatch/Props.mdx | Documents showMoreLabel and includes render-props reference table. |
| src/stories/components/ProductSwatch/ProductSwatch.stories.tsx | Adds stories demonstrating “View more” + custom label; refactors sample data. |
| src/stories/components/ProductSwatch/Code Examples.mdx | Updates examples to pass required item prop. |
| src/stories/components/ProductCard/Props.mdx | Documents new swatchConfigs prop. |
| src/stories/components/ProductCard/ProductCard.stories.ts | Adds stories for swatch limiting + custom label. |
| src/stories/components/CioPlp/Props.mdx | Documents onShowMoreSwatches and top-level swatchConfigs. |
| src/stories/components/CioPlp/Code Examples.mdx | Adds example usage for swatchConfigs. |
| src/hooks/useProductSwatch.ts | Implements maxVisibleSwatches splitting (visibleSwatches/hiddenSwatches) and derived flags. |
| src/hooks/useProduct.ts | Threads swatchConfigs.maxVisibleSwatches into useProductSwatch. |
| src/hooks/callbacks/useOnShowMoreSwatches.ts | New hook for “View more” click handling with default navigation + optional custom callback. |
| src/hooks/callbacks/index.ts | Exports the new callback hook. |
| src/components/ProductSwatch/ProductSwatch.tsx | Renders limited swatches + “View more” button; adds item/showMoreLabel props and render-props payload. |
| src/components/ProductSwatch/ProductSwatch.css | Styles the new “View more” button. |
| src/components/ProductCard/ProductCard.tsx | Passes swatchConfigs into hook and item/showMoreLabel into ProductSwatch. |
| src/components/CioPlpGrid/CioPlpGrid.tsx | Accepts and forwards swatchConfigs to ProductCard. |
| src/components/CioPlp/CioPlp.tsx | Exposes swatchConfigs as a top-level prop and forwards to grid. |
| spec/hooks/useProductSwatch/useProductSwatch.test.js | Adds tests for maxVisibleSwatches behavior. |
| spec/hooks/callbacks/useOnShowMoreSwatches.test.js | Adds tests for default navigation and custom callback behavior. |
| spec/components/ProductSwatch/ProductSwatch.test.js | Updates for required item prop and tests “View more” rendering/label/callback. |
| .storybook/storybook-styles.css | Improves docs table wrapping for long type signatures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| callback?: Callbacks['onShowMoreSwatches'], | ||
| ) { | ||
| return useCallback( | ||
| (event: React.MouseEvent) => { |
There was a problem hiding this comment.
ProductSwatch is rendered inside the <a> in ProductCard, and the “View more” control is a <button> inside that anchor. stopPropagation() prevents the card’s onClick handler, but it does not prevent the anchor default behavior in all cases. To make navigation deterministic (and to match how swatch container clicks are handled), also call event.preventDefault() in this handler before running the callback/default navigation.
| (event: React.MouseEvent) => { | |
| (event: React.MouseEvent) => { | |
| event.preventDefault(); |
| .cio-swatch-show-more { | ||
| background: none; | ||
| border: none; | ||
| padding: 0 16px 8px; | ||
| color: var(--cio-swatch-more-color, #333); | ||
| font-size: 12px; | ||
| font-family: inherit; | ||
| cursor: pointer; | ||
| text-decoration: underline; | ||
| } |
There was a problem hiding this comment.
The new “View more” button has hover styling but no visible focus state. Add :focus-visible styles (e.g., outline/underline offset) so keyboard users can see focus, especially since the button uses background: none and border: none.
| export type ProductSwatchProps = IncludeRenderProps< | ||
| { | ||
| swatchObject: ProductSwatchObject; | ||
| item: Item; | ||
| showMoreLabel?: string | ((count: number) => string); | ||
| }, | ||
| ProductSwatchObject | ||
| ProductSwatchRenderProps | ||
| >; |
There was a problem hiding this comment.
item is now a required prop for ProductSwatch, which is a breaking change for any consumers rendering ProductSwatch directly (outside ProductCard). If ProductSwatch is part of the public API, consider making item optional and only requiring it for the default “View more” navigation/render-props payload (with a clear fallback behavior when omitted), or document this as a breaking change requiring a major version bump.
| const sampleItem = { | ||
| itemName: 'Sample Product', | ||
| itemId: 'sample-1', | ||
| url: 'https://example.com/product', | ||
| data: {}, | ||
| } as Item; |
There was a problem hiding this comment.
The story uses as Item to bypass type checking, which can hide missing required Item fields and make stories drift from real runtime behavior. Prefer constructing a fully valid Item (or using the same transformResultItem helper used elsewhere in stories/tests) so Storybook examples stay representative and type-safe.
| const sampleItem = { | |
| itemName: 'Sample Product', | |
| itemId: 'sample-1', | |
| url: 'https://example.com/product', | |
| data: {}, | |
| } as Item; | |
| const sampleItem: Item = { | |
| itemName: 'Sample Product', | |
| itemId: 'sample-1', | |
| url: 'https://example.com/product', | |
| data: {}, | |
| }; |
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
| const useProductInfo: UseProductInfo = ({ item, swatchConfigs }) => { | ||
| const state = useCioPlpContext(); | ||
| const productSwatch = useProductSwatch({ item }); | ||
| const productSwatchConfigs = { |
There was a problem hiding this comment.
Important Issue: productSwatchConfigs is recreated on every render, causing a new object reference each time. Since useProductSwatch has config in a destructuring (const { maxVisibleSwatches } = config ?? {}), passing a new object on every render isn't harmful for the hook itself, but if config ever ends up in a useEffect or useMemo dependency array inside useProductSwatch, this would cause unnecessary re-executions. Wrap this with useMemo to be safe and explicit:
const productSwatchConfigs = useMemo(
() => ({ maxVisibleSwatches: swatchConfigs?.maxVisibleSwatches }),
[swatchConfigs?.maxVisibleSwatches],
);| if (callback) { | ||
| callback(event, item, selectedSwatch, hiddenSwatches, setUrl); | ||
| } else { | ||
| const url = selectedSwatch?.url || item.url; |
There was a problem hiding this comment.
Important Issue: Using || here will also treat an empty string '' as falsy and fall back to item.url. This is likely unintentional — prefer ?? if only undefined/null should trigger the fallback, or add an explicit check:
const url = selectedSwatch?.url != null ? selectedSwatch.url : item.url;| data-testid='cio-swatch-show-more' | ||
| className='cio-swatch-show-more' | ||
| onClick={showMoreClickHandler} | ||
| aria-label={`Show ${hiddenSwatches?.length} more`}> |
There was a problem hiding this comment.
Important Issue: When hasMoreSwatches is true, hiddenSwatches is guaranteed non-undefined (from useProductSwatch), but the aria-label still uses optional chaining (hiddenSwatches?.length), which can produce the string "Show undefined more" if the component is used in isolation with a manually-crafted swatchObject where hasMoreSwatches: true but hiddenSwatches: undefined. Either assert non-nullability or use the already-computed getShowMoreLabel() output for the aria-label to keep them consistent:
aria-label={`Show ${hiddenSwatches?.length ?? 0} more swatches`}| e.preventDefault(); | ||
| }; | ||
|
|
||
| const getShowMoreLabel = () => { |
There was a problem hiding this comment.
Suggestion: getShowMoreLabel is a plain function defined inside the component body and will be recreated on every render. It only depends on showMoreLabel and hiddenSwatches?.length. Consider wrapping it in useCallback or converting it to a derived constant via useMemo to keep the component's render profile lean:
const showMoreLabelText = useMemo(() => {
const hiddenCount = hiddenSwatches?.length ?? 0;
if (typeof showMoreLabel === 'function') return showMoreLabel(hiddenCount);
return showMoreLabel ?? 'View more >';
}, [showMoreLabel, hiddenSwatches?.length]);| setSelectedVariation(swatch); | ||
| }; | ||
|
|
||
| const visibleSwatches = useMemo(() => { |
There was a problem hiding this comment.
Suggestion: The visibleSwatches and hiddenSwatches memos share identical guard logic (if (!swatchList || maxVisibleSwatches === undefined)). Consider consolidating them into a single useMemo that computes both at once to avoid iterating swatchList twice and to keep the logic in one place:
const { visibleSwatches, hiddenSwatches } = useMemo(() => {
if (!swatchList || maxVisibleSwatches === undefined) {
return { visibleSwatches: swatchList, hiddenSwatches: undefined };
}
const visible = swatchList.slice(0, maxVisibleSwatches);
const hidden = swatchList.slice(maxVisibleSwatches);
return { visibleSwatches: visible, hiddenSwatches: hidden.length > 0 ? hidden : undefined };
}, [swatchList, maxVisibleSwatches]);
| import CioPlpGrid, { CioPlpGridProps } from '../CioPlpGrid'; | ||
|
|
||
| export type CioPlpProps = CioPlpProviderProps & UseCioPlpProps; | ||
| export type CioPlpProps = CioPlpProviderProps & |
There was a problem hiding this comment.
Suggestion: The PR author noted this is the first rendering-specific top-level prop on CioPlp and was hesitant to add it. The current approach works, but swatchConfigs is pulled out of the combined CioPlpProps type via Pick<CioPlpGridProps, 'swatchConfigs'>. If SwatchConfigs is extended in the future, CioPlpGridProps must be updated first, then CioPlpProps reflects it automatically, which is correct. However, documenting this coupling with a comment (or just re-exporting SwatchConfigs from this module) would help future maintainers understand why the grid prop bleeds into the top-level API.
| }); | ||
| }); | ||
|
|
||
| it('Should hide all swatches when maxVisibleSwatches is 0', async () => { |
There was a problem hiding this comment.
Suggestion: The edge-case test for maxVisibleSwatches: 0 correctly verifies that visibleSwatches is empty and hasMoreSwatches is true, but it doesn't assert totalSwatchCount. Since totalSwatchCount is always the full list length regardless of the limit, adding expect(totalSwatchCount).toBe(swatchList.length) here would close a small gap in coverage and prevent a regression if totalSwatchCount is accidentally computed from visibleSwatches in the future.
| expect(screen.getByTestId('cio-swatch-show-more')).toHaveTextContent('See all colors'); | ||
| }); | ||
|
|
||
| it('displays custom function label with hidden count', () => { |
There was a problem hiding this comment.
@Mudaafi Hey! here seems to be typo hidden instead of visible
PR Description
Adds the ability to configure a "Show More" button. The default behavior is to navigate to the PDP via
variation/item.data.url.Implementation
New Callback
onShowMoreSwatches- Defaults to navigating to the PDP viavariation/item.data.urlusingsetUrl.New Top-Level Config
I was hesitant to add this in because (a) this is our first rendering-specific top-level prop and (b), it's against the new pattern of compound components we've implemented in shared-components. Unfortunately, I don't see a good alternative. LMK if there's a better way to implement this.
swatchConfigs. showMoreLabel- Allow a user to specify astring/(num) -> stringthat renders as the buttonswatchConfigs.maxVisibleSwatches- Allows limiting the number of swatches to showNew CSS Class
.cio-swatch-show-more- A class to target this new button for stylingPull 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?