Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't filter product options with only one value in the VariantSelector #2113

Merged
merged 2 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .changeset/cold-foxes-draw.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
'skeleton': patch
'@shopify/hydrogen': patch
---

**Breaking change**

Previously the `VariantSelector` component would filter out options that only had one value. This is undesireable for some apps. We've removed that filter, if you'd like to retain the existing functionality, simply filter the options prop before it is passed to the `VariantSelector` component:

```diff
<VariantSelector
handle={product.handle}
+ options={product.options.filter((option) => option.values.length > 1)}
- options={product.options}
variants={variants}>
</VariantSelector>
```

Fixes [#1198](https://github.com/Shopify/hydrogen/discussions/1198)
3 changes: 2 additions & 1 deletion examples/b2b/app/routes/products.$handle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,8 @@ function ProductForm({
<div className="product-form">
<VariantSelector
handle={product.handle}
options={product.options}
// Only show options with more than one value
options={product.options.filter((option) => option.values.length > 1)}
variants={variants}
>
{({option}) => <ProductOptions key={option.name} option={option} />}
Expand Down
51 changes: 14 additions & 37 deletions packages/hydrogen/src/product/VariantSelector.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,43 +198,6 @@ describe('<VariantSelector>', () => {
`);
});

it('automatically appends options with only one value to the URL', () => {
const {asFragment} = render(
createElement(VariantSelector, {
handle: 'snowboard',
options: [
{name: 'Color', values: ['Red']},
{name: 'Size', values: ['S', 'M']},
],
children: ({option}) =>
createElement(
'div',
null,
option.values.map(({value, to}) =>
createElement('a', {key: option.name + value, href: to}, value),
),
),
}),
);

expect(asFragment()).toMatchInlineSnapshot(`
<DocumentFragment>
<div>
<a
href="/products/snowboard?Size=S&Color=Red"
>
S
</a>
<a
href="/products/snowboard?Size=M&Color=Red"
>
M
</a>
</div>
</DocumentFragment>
`);
});

it('prepends localization', () => {
vi.mocked(useLocation).mockReturnValueOnce(
fillLocation({search: '?Size=M', pathname: '/en-us/'}),
Expand Down Expand Up @@ -268,6 +231,13 @@ describe('<VariantSelector>', () => {

expect(asFragment()).toMatchInlineSnapshot(`
<DocumentFragment>
<div>
<a
href="/en-us/products/snowboard?Color=Red"
>
Red
</a>
</div>
<div>
<a
href="/en-us/products/snowboard?Size=S&Color=Red"
Expand Down Expand Up @@ -318,6 +288,13 @@ describe('<VariantSelector>', () => {

expect(asFragment()).toMatchInlineSnapshot(`
<DocumentFragment>
<div>
<a
href="/products/snowboard?Color=Red"
>
Red
</a>
</div>
<div>
<a
href="/products/snowboard?Size=S&Color=Red"
Expand Down
125 changes: 60 additions & 65 deletions packages/hydrogen/src/product/VariantSelector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,71 +64,66 @@ export function VariantSelector({
Fragment,
null,
...useMemo(() => {
return (
options
// Only show options with more than one value
.filter((option) => option?.values?.length! > 1)
.map((option) => {
let activeValue;
let availableValues: VariantOptionValue[] = [];

for (let value of option.values!) {
// The clone the search params for each value, so we can calculate
// a new URL for each option value pair
const clonedSearchParams = new URLSearchParams(
alreadyOnProductPage ? searchParams : undefined,
);
clonedSearchParams.set(option.name!, value!);

// Because we hide options with only one value, they aren't selectable,
// but they still need to get into the URL
optionsWithOnlyOneValue.forEach((option) => {
clonedSearchParams.set(option.name!, option.values![0]!);
});

// Find a variant that matches all selected options.
const variant = variants.find((variant) =>
variant?.selectedOptions?.every(
(selectedOption) =>
clonedSearchParams.get(selectedOption?.name!) ===
selectedOption?.value,
),
);

const currentParam = searchParams.get(option.name!);

const calculatedActiveValue = currentParam
? // If a URL parameter exists for the current option, check if it equals the current value
currentParam === value!
: false;

if (calculatedActiveValue) {
// Save out the current value if it's active. This should only ever happen once.
// Should we throw if it happens a second time?
activeValue = value;
}

const searchString = '?' + clonedSearchParams.toString();

availableValues.push({
value: value!,
isAvailable: variant ? variant.availableForSale! : true,
to: path + searchString,
search: searchString,
isActive: calculatedActiveValue,
variant,
});
}

return children({
option: {
name: option.name!,
value: activeValue,
values: availableValues,
},
});
})
);
return options.map((option) => {
let activeValue;
let availableValues: VariantOptionValue[] = [];

for (let value of option.values!) {
// The clone the search params for each value, so we can calculate
// a new URL for each option value pair
const clonedSearchParams = new URLSearchParams(
alreadyOnProductPage ? searchParams : undefined,
);
clonedSearchParams.set(option.name!, value!);

// Because we hide options with only one value, they aren't selectable,
// but they still need to get into the URL
optionsWithOnlyOneValue.forEach((option) => {
clonedSearchParams.set(option.name!, option.values![0]!);
});

// Find a variant that matches all selected options.
const variant = variants.find((variant) =>
variant?.selectedOptions?.every(
(selectedOption) =>
clonedSearchParams.get(selectedOption?.name!) ===
selectedOption?.value,
),
);

const currentParam = searchParams.get(option.name!);

const calculatedActiveValue = currentParam
? // If a URL parameter exists for the current option, check if it equals the current value
currentParam === value!
: false;

if (calculatedActiveValue) {
// Save out the current value if it's active. This should only ever happen once.
// Should we throw if it happens a second time?
activeValue = value;
}

const searchString = '?' + clonedSearchParams.toString();

availableValues.push({
value: value!,
isAvailable: variant ? variant.availableForSale! : true,
to: path + searchString,
search: searchString,
isActive: calculatedActiveValue,
variant,
});
}

return children({
option: {
name: option.name!,
value: activeValue,
values: availableValues,
},
});
});
}, [options, variants, children]),
);
}
Expand Down
2 changes: 1 addition & 1 deletion templates/skeleton/app/routes/products.$handle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ function ProductForm({
<div className="product-form">
<VariantSelector
handle={product.handle}
options={product.options}
options={product.options.filter((option) => option.values.length > 1)}
variants={variants}
>
{({option}) => <ProductOptions key={option.name} option={option} />}
Expand Down
Loading