Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the variant selection and pricing logic for product pages. The main architectural change is converting VariantSelector from a hybrid controlled/uncontrolled component to a fully controlled component, with state management lifted to the parent ProductDetails component. The PR also implements variant-level discount support and changes the default behavior to show product base price initially instead of pre-selecting a variant.
Changes:
- Converted
VariantSelectorto a controlled component by removing internal state and acceptingselectedVariantIdprop - Changed default variant selection behavior: no variant is pre-selected on page load (previously defaulted to first/default variant)
- Implemented variant-level discount precedence using
getEffectiveDiscountutility - Added variant toggle functionality: clicking a selected variant deselects it and returns to product base price
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/app/store/[slug]/components/variant-selector.tsx |
Removed local state management (useState, useMemo) and converted to controlled component accepting selectedVariantId prop. Added null-safe checks for selectedVariant throughout rendering logic. |
src/app/store/[slug]/components/product-details.tsx |
Removed pre-selection of default variant (now starts with null), added variant discount handling with getEffectiveDiscount, implemented toggle behavior for variant deselection, and passes selectedVariantId to child component. |
| // Toggle: if already selected, deselect it (go back to default product price) | ||
| if (selectedVariant?.id === variant.id) { | ||
| setSelectedVariant(null); | ||
| } else { | ||
| setSelectedVariant(variant); | ||
| // Update image gallery if variant has an image | ||
| if (variant.image && onImageChange) { | ||
| onImageChange(variant.image); | ||
| } |
There was a problem hiding this comment.
The toggle behavior introduced in handleVariantChange creates a confusing user experience. Clicking a selected variant now deselects it and returns to showing the product's default price, but this pattern is unexpected for product variant selection. In typical e-commerce flows, users expect variant selection to be persistent and not toggleable - they should switch between variants rather than toggle them on/off.
This behavior can be particularly confusing because:
- After deselecting, users won't know which variant will be added to the cart
- The image gallery update logic doesn't handle deselection (no reset to default product image)
- It's inconsistent with standard e-commerce UX patterns
Consider either removing the toggle behavior entirely or defaulting to the first/default variant instead of null.
| // Toggle: if already selected, deselect it (go back to default product price) | |
| if (selectedVariant?.id === variant.id) { | |
| setSelectedVariant(null); | |
| } else { | |
| setSelectedVariant(variant); | |
| // Update image gallery if variant has an image | |
| if (variant.image && onImageChange) { | |
| onImageChange(variant.image); | |
| } | |
| setSelectedVariant(variant); | |
| // Update image gallery if variant has an image | |
| if (variant.image && onImageChange) { | |
| onImageChange(variant.image); |
| setSelectedVariant(null); | ||
| } else { | ||
| setSelectedVariant(variant); | ||
| // Update image gallery if variant has an image | ||
| if (variant.image && onImageChange) { | ||
| onImageChange(variant.image); | ||
| } |
There was a problem hiding this comment.
When a variant is deselected (toggled off), the image gallery is not reset to the default product image. The current logic only handles updating the image when a variant is selected, but when setSelectedVariant(null) is called, the image remains on the last selected variant's image rather than reverting to the product's default image.
To fix this, add an else branch that resets the image when deselecting, or call onImageChange with the product's default thumbnail when setting the variant to null.
| const variantDiscount: DiscountInfo | null = selectedVariant ? { | ||
| discountType: selectedVariant.discountType ?? null, | ||
| discountValue: selectedVariant.discountValue ?? null, | ||
| discountStartDate: selectedVariant.discountStartDate ? new Date(selectedVariant.discountStartDate) : null, | ||
| discountEndDate: selectedVariant.discountEndDate ? new Date(selectedVariant.discountEndDate) : null, | ||
| } : null; | ||
|
|
||
| // Use variant discount if active, otherwise fall back to product discount | ||
| const effectiveDiscount = getEffectiveDiscount(productDiscount, variantDiscount); | ||
|
|
There was a problem hiding this comment.
When no variant is selected (selectedVariant is null), the discount calculation falls back to productDiscount, but currentPrice also falls back to product.price. This creates a potential issue: if a variant has a different price than the product, the discount might be calculated incorrectly when no variant is selected.
For example, if the product has price 100 with 10% discount, but all variants have price 200 with no discount, showing "90" (discounted product price) when no variant is selected could be misleading if users can only purchase variants, not the base product.
Verify that the pricing logic aligns with the business requirements - should the product's base price and discount be shown when variants exist but none is selected?
| const variantDiscount: DiscountInfo | null = selectedVariant ? { | |
| discountType: selectedVariant.discountType ?? null, | |
| discountValue: selectedVariant.discountValue ?? null, | |
| discountStartDate: selectedVariant.discountStartDate ? new Date(selectedVariant.discountStartDate) : null, | |
| discountEndDate: selectedVariant.discountEndDate ? new Date(selectedVariant.discountEndDate) : null, | |
| } : null; | |
| // Use variant discount if active, otherwise fall back to product discount | |
| const effectiveDiscount = getEffectiveDiscount(productDiscount, variantDiscount); | |
| const variantDiscount: DiscountInfo | null = selectedVariant | |
| ? { | |
| discountType: selectedVariant.discountType ?? null, | |
| discountValue: selectedVariant.discountValue ?? null, | |
| discountStartDate: selectedVariant.discountStartDate ? new Date(selectedVariant.discountStartDate) : null, | |
| discountEndDate: selectedVariant.discountEndDate ? new Date(selectedVariant.discountEndDate) : null, | |
| } | |
| : null; | |
| // When variants exist and none is selected, do not show a product-level discount, | |
| // to avoid advertising a price that may not be available on any purchasable variant. | |
| const baseDiscountForDisplay: DiscountInfo = | |
| variants.length > 0 && !selectedVariant | |
| ? { | |
| discountType: null, | |
| discountValue: null, | |
| discountStartDate: null, | |
| discountEndDate: null, | |
| } | |
| : productDiscount; | |
| // Use variant discount if active, otherwise fall back to the base discount for display | |
| const effectiveDiscount = getEffectiveDiscount(baseDiscountForDisplay, variantDiscount); |
| <Label> | ||
| Variant:{" "} | ||
| <span className="font-semibold">{selectedVariant.name}</span> | ||
| <span className="font-semibold">{selectedVariant ? selectedVariant.name : "Select a variant"}</span> |
There was a problem hiding this comment.
The variant selector now displays "Select a variant" when no variant is selected, which is good UX. However, there's a mismatch with the new behavior in product-details.tsx where no variant is pre-selected by default.
Consider the user flow: when a product page loads with variants, users see "Select a variant" in the selector, but the price displayed above is the product's default price (potentially with discount). If variants are required for purchase, this could be confusing. Consider either:
- Pre-selecting the default variant (reverting to the old behavior)
- Showing a message like "Price varies by variant - please select" when no variant is selected
- Ensuring the AddToCartButton is disabled when variants exist but none is selected
| <span className="font-semibold">{selectedVariant ? selectedVariant.name : "Select a variant"}</span> | |
| <span className="font-semibold"> | |
| {selectedVariant | |
| ? selectedVariant.name | |
| : "Price varies by variant\u00a0-\u00a0please select"} | |
| </span> |
No description provided.