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.

Sign up for GitHub

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

fix(template-retail-react-app): product-list refinements #957

Merged
merged 6 commits into from
Mar 15, 2023
Merged

fix(template-retail-react-app): product-list refinements #957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d72eae0
fix(template-retail-react-app): product-list refinements
breadadams Feb 2, 2023
026123a
Merge branch 'develop' into fix/product-list-refinements
bendvc Mar 1, 2023
1852005
Merge branch 'develop' into fix/product-list-refinements
bendvc Mar 3, 2023
f26ba60
Merge branch 'develop' into fix/product-list-refinements
bendvc Mar 7, 2023
68dfadf
feat(retail-react-app): update `CheckboxRefinements`
breadadams Mar 15, 2023
4d0e4bb
Merge remote-tracking branch 'upstream/develop' into fix/product-list…
breadadams Mar 15, 2023
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: 14 additions & 5 deletions packages/template-retail-react-app/app/pages/product-list/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,25 +195,34 @@ const ProductList = (props) => {
// If we aren't allowing for multiple selections, simply clear any value set for the
// attribute, and apply a new one if required.
if (!allowMultiple) {
const previousValue = searchParamsCopy.refine[attributeId]
delete searchParamsCopy.refine[attributeId]

if (!selected) {
// Note the loose comparison, for "string != number" checks.
if (!selected && value.value != previousValue) {
searchParamsCopy.refine[attributeId] = value.value
}
} else {
// Get the attibute value as an array.
let attributeValue = searchParamsCopy.refine[attributeId] || []
let values = Array.isArray(attributeValue) ? attributeValue : attributeValue.split('|')

// Ensure that the value is still converted into an array if it's a `string` or `number`.
if (typeof attributeValue === 'string') {
attributeValue = attributeValue.split('|')
} else if (typeof attributeValue === 'number') {
attributeValue = [attributeValue]
}
Comment on lines +209 to +214
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally the number values in the query string were not being parsed as numbers and were being treated as strings. Looks like we broke this logic after making the change to parse numbers.

Thanks for catching that. 👍


// Either set the value, or filter the value out.
if (!selected) {
values.push(value.value)
attributeValue.push(value.value)
} else {
values = values?.filter((v) => v !== value.value)
// Note the loose comparison, for "string != number" checks.
attributeValue = attributeValue?.filter((v) => v != value.value)
}

// Update the attribute value in the new search params.
searchParamsCopy.refine[attributeId] = values
searchParamsCopy.refine[attributeId] = attributeValue

// If the update value is an empty array, remove the current attribute key.
if (searchParamsCopy.refine[attributeId].length === 0) {
Expand Down
13 changes: 4 additions & 9 deletions packages/template-retail-react-app/app/pages/product-list/partials/checkbox-refinements.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,13 @@ const CheckboxRefinements = ({filter, toggleFilter, selectedFilters}) => {
{filter.values
?.filter((refinementValue) => refinementValue.hitCount > 0)
.map((value) => {
const isChecked = selectedFilters.includes(value.value)

return (
<Box key={value.value}>
<Checkbox
isChecked={!!selectedFilters}
onChange={() =>
toggleFilter(
value,
filter.attributeId,
!!selectedFilters,
false
)
}
isChecked={isChecked}
onChange={() => toggleFilter(value, filter.attributeId, isChecked)}
>
{value.label}
</Checkbox>
Expand Down
22 changes: 7 additions & 15 deletions packages/template-retail-react-app/app/pages/product-list/partials/color-refinements.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,28 +21,20 @@ const ColorRefinements = ({filter, toggleFilter, selectedFilters}) => {
{filter.values
.filter((refinementValue) => refinementValue.hitCount > 0)
.map((value, idx) => {
const isSelected = selectedFilters.includes(value.value)

return (
<Box key={idx}>
<HStack
onClick={() =>
toggleFilter(
value,
filter.attributeId,
selectedFilters?.includes(value.value)
)
}
onClick={() => toggleFilter(value, filter.attributeId, isSelected)}
spacing={1}
cursor="pointer"
>
<Button
{...styles.swatch}
color={
selectedFilters?.includes(value.value)
? 'black'
: 'gray.200'
}
border={selectedFilters?.includes(value.value) ? '1px' : '0'}
aria-checked={selectedFilters?.includes(value.value)}
color={isSelected ? 'black' : 'gray.200'}
border={isSelected ? '1px' : '0'}
aria-checked={isSelected}
variant="outline"
marginRight={0}
marginBottom="-1px"
Expand Down Expand Up @@ -90,7 +82,7 @@ const ColorRefinements = ({filter, toggleFilter, selectedFilters}) => {
ColorRefinements.propTypes = {
filter: PropTypes.object,
toggleFilter: PropTypes.func,
selectedFilters: PropTypes.oneOfType([PropTypes.array, PropTypes.string])
selectedFilters: PropTypes.array
}

export default ColorRefinements
8 changes: 6 additions & 2 deletions packages/template-retail-react-app/app/pages/product-list/partials/radio-refinements.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import PropTypes from 'prop-types'
const RadioRefinements = ({filter, toggleFilter, selectedFilters}) => {
return (
<Box>
<RadioGroup value={selectedFilters}>
<RadioGroup
// The following `false` fallback is required to avoid the radio group
// from switching to "uncontrolled mode" when `selectedFilters` is empty.
value={selectedFilters[0] ?? false}
Copy link
Contributor Author

@breadadams breadadams Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key to fixing item 6 from the points listed in the description.

If RadioGroup receives undefined or null in the value prop, it no longer acts in controlled mode and doesn't handle the checked state of inner radio buttons.

This could cause issues if a refinement had an option where .value is equal to false. In that case the false fallback would just need to be replaced with any other non-null value (e.g. '').

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

>
<Stack spacing={1}>
{filter.values
.filter((refinementValue) => refinementValue.hitCount > 0)
Expand All @@ -28,7 +32,7 @@ const RadioRefinements = ({filter, toggleFilter, selectedFilters}) => {
toggleFilter(
value,
filter.attributeId,
selectedFilters?.includes(value.value),
selectedFilters.includes(value.value),
false
)
}
Expand Down
8 changes: 7 additions & 1 deletion packages/template-retail-react-app/app/pages/product-list/partials/refinements.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,13 @@ const Refinements = ({filters, toggleFilter, selectedFilters, isLoading}) => {
{filters?.map((filter, idx) => {
// Render the appropriate component for the refinement type, fallback to checkboxes
const Values = componentMap[filter.attributeId] || CheckboxRefinements
const selectedFiltersArray = selectedFilters?.[filter.attributeId]
let selectedFiltersArray = selectedFilters?.[filter.attributeId] ?? []

// Catch any non-array values and wrap them in an array
if (!Array.isArray(selectedFiltersArray)) {
selectedFiltersArray = [selectedFiltersArray]
}

if (filter.values) {
return (
<Stack key={filter.attributeId} divider={<Divider />}>
Expand Down
19 changes: 9 additions & 10 deletions packages/template-retail-react-app/app/pages/product-list/partials/size-refinements.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,20 @@ const SizeRefinements = ({filter, toggleFilter, selectedFilters}) => {
{filter.values
?.filter((refinementValue) => refinementValue.hitCount > 0)
.map((value, idx) => {
const selected = Array.isArray(selectedFilters)
? selectedFilters?.includes(value.value)
: selectedFilters === value.value
// Note the loose comparison, for "string == number" checks.
const isSelected = selectedFilters.some(
(filterValue) => filterValue == value.value
)

return (
<Button
key={idx}
{...styles.swatch}
borderColor={selected ? 'black' : 'gray.200'}
backgroundColor={selected ? 'black' : 'white'}
color={selected ? 'white' : 'gray.900'}
onClick={() => toggleFilter(value, filter.attributeId, selected)}
aria-checked={selectedFilters == value.value}
borderColor={isSelected ? 'black' : 'gray.200'}
backgroundColor={isSelected ? 'black' : 'white'}
color={isSelected ? 'white' : 'gray.900'}
onClick={() => toggleFilter(value, filter.attributeId, isSelected)}
aria-checked={isSelected}
variant="outline"
marginBottom={0}
marginRight={0}
Expand All @@ -47,8 +48,6 @@ const SizeRefinements = ({filter, toggleFilter, selectedFilters}) => {

SizeRefinements.propTypes = {
filter: PropTypes.object,
selectedFilterValues: PropTypes.object,
categoryId: PropTypes.string,
selectedFilters: PropTypes.array,
toggleFilter: PropTypes.func
}
Expand Down