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

breadadams · 2023-02-02T01:53:55Z

Description

This PR fixes numerous points opened in #901 regarding the PLP refinements component, such as:

Having numerous possible types (string | number | Array<string | number>) for the filter values passed down to a value component (color, checkbox, etc), and then running methods like .includes() against them, providing different results depending on the type.
Invalid isChecked logic for checkboxes, making all checkboxes in a refinement group appear as checked after selecting just one.
Confusing string|array proptype on the color refinements component.
Invalid handling of query parameters, where a number is received but it was then treated as a string (by attempting to access the .split() method on it).

In addition to a couple of other bug fixes:

Not being able to deselect a "non multiple" refinement (e.g. checkbox) by unchecking the selected checkbox (as it would select itself again). This was introduced (and resolved) in this PR after having fixed item 2 from above.
Radio groups maintaining their previously checked UI after having their value cleared via the "Selected Refinements" clear filter buttons.

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

Removing a public function or component or prop

Adding a required argument to a function

Changing the data type of a function parameter or return value

Adding a new peer dependency to package.json

Changes

See details in commit message and description above.

How to Test-Drive This PR

Steps for testing the (testable) points described above:

Item 1

If you potentially had a refinement option that was a substring of another, for instance color, "blue" and "light blue". When the selected value was "light blue", both would appear as selected since here it would be using the string.includes() method, instead of array.includes().

Item 2

Visit the electronics category.
Select the "Type → Lithium Ion" refinement checkbox.
Both "Lithium Ion" and "Lithium Polymer" will be checked.

Item 4

Visit the mens suits category.
Select the "Size → 36" refinement option.
(Note it's not marked as selected visually).
Select "Size → 38".
Nothing happens, see an error in the browser console.

Item 6

Visit the electronics category.
Select the "Price → $20 - $49.99" refinement radio option.
Clear the refinement option via the individual clear buttons or the "Clear all" button in the "Selected refinements" area.
Notice that the radio option selected in step 2 is still visually checked.

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

+ Ensure that the `selectedFilters` provided to the `Refinements` value components is always an array. And update said value components (checkbox, color, radio & size) to handle always receiving an array. + Apply some loose equality checks, catering for cases where a refinement URL param is parsed as a number but the refinement `.value` is a stringified number. + Fix bug where non-multiple filters could not be unchecked by selecting themselves (e.g. clicking an already checked checkbox). + Fix bug where a refinement URL param is parsed as a number, but was handled as a string (with `.split()`). + Fix bug where radio refinements would still appear as checked after clearing the value via `SelectedRefinements`. + Deprecate unused proptypes in `SizeRefinements`

breadadams · 2023-02-02T01:59:49Z

packages/template-retail-react-app/app/pages/product-list/partials/radio-refinements.jsx

+            <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}


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. '').

Nice catch.

breadadams · 2023-02-02T02:04:38Z

packages/template-retail-react-app/app/pages/product-list/partials/checkbox-refinements.jsx

-                                        !!selectedFilters,
-                                        false
-                                    )
+                                    toggleFilter(value, filter.attributeId, isChecked, false)


I didn't apply it yet, but whilst making these changes on the project I'm working on I've also removed the final false argument (allowMultiple) from the toggleFilter method. Thus making it take the default of true.

This being because IMO checkboxes should represent an option that can have multiple items selected (and be deselected). Whereas currently they're essentially serving as radio buttons.

If you agree I can remove it here too.

I'm open to this based on your reasoning. But that final argument is being used used when called from the radio-refinements component. Is your change away of this and retain the correct functionality in that component?

Hey @bendvc, yeah it is, sorry my comment here wasn't very clear.

I don't mean removing the argument from the actual toggleFilter method, just removing the false argument here where it's called within CheckboxRefinements (so that the original method's default value is applied).

@breadadams Ok 💯 makes sense. I'm ok with that change. Go ahead and commit that, and we can get this merged.

@bendvc, great! Resolved in latest commit 🙂

vmarta · 2023-02-03T01:34:50Z

Thank you @breadadams for this PR. I plan to review it tomorrow.

Update: sorry my bad, I haven't had a chance to look at this yet. Will get back to this as soon as I can.

bendvc · 2023-03-07T21:25:31Z

packages/template-retail-react-app/app/pages/product-list/index.jsx

+            // 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]
+            }


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. 👍

bendvc · 2023-03-07T21:40:56Z

Hey @breadadams .. thanks for the contribution. I've added some replies to you comments. Lets work on getting that resolved and we can merged your PR 👍

+ Adjust the `onChange` logic so that multiple checkbox refinements can be selected

…-refinements

bendvc

Hey Brad, Really appreciate the work for this fix. Everything looks a-ok 👍

* Update `develop` with `release-v2.7.0` (#1033) * Starting release process for 2.7.0 * Update Create MRT credentials file (#1025) * Release 2.7: fix order of hooks (#1027) * Return early only after all of the hooks are called * Bumper version --------- Co-authored-by: Ben Chypak <[email protected]> * Quick fix for einstein data (#1028) * Quick fix for einstein data * Bump Version * Re-lock package-lock files with npm 8 * Update package-lock.json * Update einstein.js * Regenerate lock files (#1030) * Regenerate lock files * Bump version to 2.7.0-alpha.3 * Bump to 2.7.0 (#1032) * Begin development on 2.8.0 --------- Co-authored-by: Adam Raya <[email protected]> Co-authored-by: Vincent Marta <[email protected]> * Move the MRT reference app to the SDKs, so that we can verify eg. Node support (#966) * BUG: Changed type of the phone number field to bring up numberic keyboard on mobile devices - W-9871940 (#1016) Co-authored-by: Ben Chypak <[email protected]> * Update Retail React App Page Designer integration README (#1041) Co-authored-by: Richard Sexton <[email protected]> * Implement `updateCustomerPassword` as no-op. (#1031) * Allow query hook parameters to be `null`. (#1046) * Remove unused util. * Allow query hook parameters to be `null`. * Fix addresses not having preferred address first. (#1051) * Fix addresses not having preferred address first. * Include all addresses, not just preferred address twice. * Correctly include preferred address. * Make `mergeBasket` Conditional More Robust (#1048) * Update merge logic * Update CHANGELOG.md * Lint * PR feedback * Rename isNewlyRegisters to isNew for simplicity * Lint * [commerce-sdk-react] Decode pre-fetched token and save auth data in storage (#1052) * add test * decode jwt data * lint * Update packages/commerce-sdk-react/src/auth/index.ts Co-authored-by: Will Harney <[email protected]> * rename parseSlasJWT * Update packages/commerce-sdk-react/src/auth/index.test.ts Co-authored-by: Will Harney <[email protected]> * convert all .thens to await * make fake token in tests more clear * lint --------- Co-authored-by: Will Harney <[email protected]> * Prevent modal to open when it fails to add an item to cart (#1053) * prevent modal to open when it fails to add an item to cart * Update comment --------- Co-authored-by: Alex Vuong <[email protected]> * Fix `getConfig` referencing config from incorrect location (#1049) * Initial Commit * Update CHANGELOG.md * Revert some testing code * Get test coverage back up * Test build directory folder before proceeding. * fix(template-retail-react-app): product-list refinements (#957) * fix(template-retail-react-app): product-list refinements + Ensure that the `selectedFilters` provided to the `Refinements` value components is always an array. And update said value components (checkbox, color, radio & size) to handle always receiving an array. + Apply some loose equality checks, catering for cases where a refinement URL param is parsed as a number but the refinement `.value` is a stringified number. + Fix bug where non-multiple filters could not be unchecked by selecting themselves (e.g. clicking an already checked checkbox). + Fix bug where a refinement URL param is parsed as a number, but was handled as a string (with `.split()`). + Fix bug where radio refinements would still appear as checked after clearing the value via `SelectedRefinements`. + Deprecate unused proptypes in `SizeRefinements` * feat(retail-react-app): update `CheckboxRefinements` + Adjust the `onChange` logic so that multiple checkbox refinements can be selected --------- Co-authored-by: Ben Chypak <[email protected]> * update docs for shopper-experience scope (#1059) * add docs for experience scope * Add `merge_group` event to trigger GA builds --------- Co-authored-by: Will Harney <[email protected]> Co-authored-by: Adam Raya Navarro <[email protected]> * Update lockfiles to reflect current version. (#1071) * [commerce-sdk-react] Implement remaining Shopper Baskets cache logic (#1070) * Refactor Shopper Basket cache following new pattern * Fix types * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/cache.ts Co-authored-by: Will Harney <[email protected]> * PR Feedback * Clean up unused utils and types after refactor * Add missing response basketId to queryKey * Implement cache for the remaining mutations * Tests WIP * PR Feedback & Use query key path in `deleteBasket` cache * Add tests for mutations returning void response * PR Feedback * Remove unused `and` hooks util * Revert "Remove unused `and` hooks util" This reverts commit c0a364a. --------- Co-authored-by: Will Harney <[email protected]> * [commerce-sdk-react] Fix Shopper Baskets Test case (#1082) * Fix Shopper Basket empty response test cases * lint * feat(pwa-kit-dev): minor performance improvements and added comments (#974) * feat(pwa-kit-dev): minor performance improvements and added comments * docs(pwa-kit-dev): clean up comments * refactor(pwa-kit-dev): update condition from PR feedback --------- Co-authored-by: Will Harney <[email protected]> * Update dependencies. (#1079) * Remove internal packages to bypass lerna nonsense. Include output.json to make restoring deps easier. * Disable build scripts to make changing deps easier. * Update root deps. * Fix typescript issue. * Create restore script to restore internal deps/scripts. * Update commerce-sdk-react deps. * Update test-commerce-sdk-react deps. * Update template-typescript-minimal deps * Improve restore script. * Fix trailing comma. * Update template-retail-react-app deps * npm prune everything * Update pwa-kit-runtime deps * Update pwa-kit-react-sdk deps * Update pwa-kit-create-app deps * Update pwa-kit-dev deps (except eslint) * Update pwa-kit-dev eslint deps * Update internal-lib-build deps (except eslint) * Update pwa-kit eslint deps * Restore internal deps. * Restore build scripts. * Remove temporary helper files. * Bump ua-parser-js to avoid vulnerable version. * Anchor cross-env common dep to version used by internal-lib-build. * Re-enable eslint. * Implement Cache Logic for Shopper APIs (Contexts/Customers/Login/Orders) (#1073) * Initial commit * Update packages/commerce-sdk-react/src/hooks/useAuthHelper.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts Co-authored-by: Will Harney <[email protected]> * Added root to all query keys, use remove over clear * Remove previous impemented "clear" from utils * Initial tests for shoppercontexts * Update ShopperLogin tests * Fix order tests * Update packages/commerce-sdk-react/src/hooks/ShopperContexts/cache.ts Co-authored-by: Will Harney <[email protected]> * Update cache.ts * Lint! * Update Json.tsx * Lint! * Testing race condition in tests * Re-add tests in other order. * Update CHANGELOG.md * Add todo to complete context cache work * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/mutation.test.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/components/ShopperExperience/Page/index.tsx Co-authored-by: Will Harney <[email protected]> * Update useAuthHelper.ts * Update packages/commerce-sdk-react/src/hooks/ShopperCustomers/cache.ts Co-authored-by: Adam Raya <[email protected]> --------- Co-authored-by: Will Harney <[email protected]> Co-authored-by: Adam Raya <[email protected]> * remove site alias and locale from location.state.directedFrom path (#1065) * remove site alias and locale from location.state.directedFrom path * moving call to removeSiteLocaleFromPath into use-navigation hook * fixing failing test, added tests for removeSiteLocaleFromPath * per code review, skipping failed test instead of using mocking * Fix Page Designer ImageWithText Link component (#1092) * Fix PD ImageWithText Link component by using Chakra UI Link * Use isAbsoluteURL to use react-router Link or Chakra Link component * PR Feedback * Clean up * Update packages/template-retail-react-app/app/page-designer/assets/image-with-text/index.jsx Co-authored-by: Ben Chypak <[email protected]> * Remove temporal page-viewer page to facilitate review --------- Co-authored-by: Ben Chypak <[email protected]> * split ssr build on local (#1155) * add suffix to ssr build files (#1157) * Added session bridge call to login for phased launch (#1159) * Added session bridge call to login for phased launch * Fix code smell for session-bridge in hybrid * Fix multi-value query params being lost (#1150) * Fix multi-value query params being lost * Update CHANGELOG.md * Snyk dependency updates (#1169) * Dependency updates * Update runtime package lock * Bump cosmiconfig version to latest * [Hybrid] PWA Kit should have a mechanism for replacing the access token when a SFRA login state is changed (#1171) * Implement mechanism to store refresh token copy and compare with sfra * Update tests and mocks for util function to check SFRA login state * Fix linting issues * FIx param types for util functionn * Rename old isTokenValid to isTokenExpired * Remove expiry for refresh_token in localstorage * Update packages/template-retail-react-app/app/commerce-api/utils.js Co-authored-by: Kevin He <[email protected]> * fix test * Fix linting on use-auth-modal.test.js * Update hasSFRAStateChanged logic to compare keys and values * Fix linting --------- Co-authored-by: Kevin He <[email protected]> * Add a redirect to login page after user signs out from checkout page (#1172) * Add a redirect to login page after user signs out from checkout page * Update CHANGELOG.md * Remove history since navigate handles similarly * Bump version number to 2.7.1-alpha.0 * Update changelogs * #1174 Replace invalid value for wrap property (#1179) * Update changelogs * Version bump to 2.7.1-preview.0 * Revert "Version bump to 2.7.1-preview.0" This reverts commit 985a7e0. * Update CHANGELOG.md * Rebuild lock files and fix ShopperLogin mutation test * Revert "Rebuild lock files and fix ShopperLogin mutation test" This reverts commit d9cfe50. * Add additional properties to ShopperLogin test types (#1185) * [V2] Re-generate lock files and fix hook lib tests (#1186) * re-generate lock files and fix test * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/index.test.ts * Rebuild lockfiles using node 14 npm 8 * Revert "Rebuild lockfiles using node 14 npm 8" This reverts commit 3d5c0cb. * Use pwa-kit-dev for lint and format * Revert "Use pwa-kit-dev for lint and format" This reverts commit f46d83e. * Add typescript to internal-lib-build and rebuild lock files --------- Co-authored-by: vcua-mobify <[email protected]> Co-authored-by: vcua-mobify <[email protected]> * [V2] Internal lib build typescript dev dependency (#1194) * Move typescript to dev and peer dependency * Update package lock file * Lockfile updates --------- Co-authored-by: Ben Chypak <[email protected]> Co-authored-by: Adam Raya <[email protected]> Co-authored-by: Vincent Marta <[email protected]> Co-authored-by: Oliver Brook <[email protected]> Co-authored-by: echessman <[email protected]> Co-authored-by: John Boxall <[email protected]> Co-authored-by: Richard Sexton <[email protected]> Co-authored-by: Will Harney <[email protected]> Co-authored-by: Kevin He <[email protected]> Co-authored-by: Alex Vuong <[email protected]> Co-authored-by: Brad Adams <[email protected]> Co-authored-by: Charles Lavery <[email protected]> Co-authored-by: Adam Raya Navarro <[email protected]> Co-authored-by: ecRobertEngel <[email protected]> Co-authored-by: Sandra Golden <[email protected]> Co-authored-by: Jainam Sheth <[email protected]> Co-authored-by: mdenchev-aiopsgroup <[email protected]>

breadadams requested a review from a team as a code owner February 2, 2023 01:53

salesforce-cla bot added the cla:signed label Feb 2, 2023

breadadams commented Feb 2, 2023

View reviewed changes

vmarta added the Acknowledged Team has responded to issue label Feb 3, 2023

bendvc added 3 commits March 1, 2023 14:20

Merge branch 'develop' into fix/product-list-refinements

026123a

Merge branch 'develop' into fix/product-list-refinements

1852005

Merge branch 'develop' into fix/product-list-refinements

f26ba60

bendvc reviewed Mar 7, 2023

View reviewed changes

breadadams added 2 commits March 15, 2023 01:48

feat(retail-react-app): update CheckboxRefinements

68dfadf

+ Adjust the `onChange` logic so that multiple checkbox refinements can be selected

Merge remote-tracking branch 'upstream/develop' into fix/product-list…

4d0e4bb

…-refinements

bendvc approved these changes Mar 15, 2023

View reviewed changes

bendvc merged commit cd4e64f into SalesforceCommerceCloud:develop Mar 15, 2023

bfeister mentioned this pull request Apr 12, 2023

[BUG] CheckboxRefinements selects all checkboxes when refining #901

Closed

vcua-mobify mentioned this pull request May 9, 2023

PWA Kit 2.7.1-alpha.0 release #1181

Merged

vcua-mobify mentioned this pull request May 11, 2023

[V2] PWA Kit Release 2.7.1 #1197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

breadadams commented Feb 2, 2023

breadadams Feb 2, 2023 •

edited

Loading

bendvc Mar 7, 2023

breadadams Feb 2, 2023

bendvc Mar 7, 2023

breadadams Mar 9, 2023

bendvc Mar 10, 2023

breadadams Mar 15, 2023

vmarta commented Feb 3, 2023 •

edited

Loading

bendvc Mar 7, 2023

bendvc commented Mar 7, 2023

bendvc left a comment

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

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

Conversation

breadadams commented Feb 2, 2023

Description

Types of Changes

Changes

How to Test-Drive This PR

Item 1

Item 2

Item 4

Item 6

Checklists

General

Accessibility Compliance

Localization

breadadams Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

bendvc Mar 7, 2023

Choose a reason for hiding this comment

breadadams Feb 2, 2023

Choose a reason for hiding this comment

bendvc Mar 7, 2023

Choose a reason for hiding this comment

breadadams Mar 9, 2023

Choose a reason for hiding this comment

bendvc Mar 10, 2023

Choose a reason for hiding this comment

breadadams Mar 15, 2023

Choose a reason for hiding this comment

vmarta commented Feb 3, 2023 • edited Loading

bendvc Mar 7, 2023

Choose a reason for hiding this comment

bendvc commented Mar 7, 2023

bendvc left a comment

Choose a reason for hiding this comment

breadadams Feb 2, 2023 •

edited

Loading

vmarta commented Feb 3, 2023 •

edited

Loading