@W-18767088 refactor add to cart#2664
@W-18767088 refactor add to cart#2664sf-kyle-wright merged 6 commits intostandard-product-supportfrom
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
sf-emmyzhang
left a comment
There was a problem hiding this comment.
Assuming we want to move the unit tests over too?
Ya I'll do that in a few. Wanted to get feedback on the approach |
| */ | ||
|
|
||
| /** | ||
| * Handles adding products to cart and sending data to Einstein. |
There was a problem hiding this comment.
@sf-kyle-wright The WI description mentioned one reason to move addToCart out is it can be used from multiple clients like PDP, wishlist etc.
Then utils/cart-utils does not seem the right place to me.
Should this be a hook or something like that instead ?
Hooks can contain logic for data fetching, state management, UI behavior etc.
There was a problem hiding this comment.
Ya so that's a great thought. There are some tradeoffs here but it looks like at this point it might not make sense to move them into a hook. I asked cursor, TLDR:
Summary:
It doesn't strictly make sense to turn these into a hook unless you want to manage state or context, or provide a more React-friendly API. If you do, a hook can be a good abstraction, but the current logic itself doesn't require it.
| } | ||
|
|
||
| /**************** Product Set/Bundles Handlers ****************/ | ||
| const handleChildProductValidation = useCallback(() => { |
There was a problem hiding this comment.
Do we need to move this as well ? If Yes, this can go in some util file.
Others can go in some AddToCart hook/or something, as they are all Adding diff products to cart.
There was a problem hiding this comment.
handleChildProductValidation - Moved it back to product-detail since it didn't make sense to extract this one to the util
|
There are currently no tests that directly test handleAddToCart, handleProductBundleAddToCart, or handleProductSetAddToCart. I will create dedicated tests for each of these in a follow up PR |
…factor-add-to-cart Signed-off-by: Kyle Wright <30636085+sf-kyle-wright@users.noreply.github.com>
|
Fair warning, this refactor isn't compatible with BOPIS, add to cart needs much more data passed in. see https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2646/files#r2170144721 |
| */ | ||
|
|
||
| /** | ||
| * Handles adding products to cart and sending data to Einstein. |
There was a problem hiding this comment.
since this is only working on cart? How about we keep it in cart page? Unless we have plan for these funcs to be used by other components that is not cart
There was a problem hiding this comment.
There is a larger plan to re-use these functions across wishlist as well
There was a problem hiding this comment.
if that is the case, let's name the file to be a bit more generic.
There was a problem hiding this comment.
thanks. renamed to add-to-cart-utils.js
Description
Extracting existing add to cart functions from pdp into re-usable util file
Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization