[PWA-413] [Cart Page Edit] Price Does not get updated after changing the color/size. #2483

tjwiebell · 2020-06-10T21:27:59Z

Description

Steps -

Add a configurable product to Cart and go Cart page.
Go to Edit panel and switch to different color.

Expected - Price should get updated based on selection.
Actual - Price is static, user will get confused after seeing the update on Cart page.

Note - Same issue exists on Mini Cart Edit window as well but will be part of mini cart refactor?

Related Issue

[PWA-413] [Cart Page Edit] Price Does not get updated after changing the color/size.

Acceptance

Verification Stakeholders

Specification

Verification Steps

⬆️ Use repro steps in description ⬆️

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.

I have updated the documentation accordingly, if necessary.

…nts to show current selected price.

PWAStudioBot · 2020-06-10T21:40:24Z

	Fails
🚫	A version label is required. A maintainer must add one.

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-413.

Generated by 🚫 dangerJS against 3927f86

devops-pwa-codebuild · 2020-06-10T21:48:04Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2483.pwa-venia.com : LH Performance Expected 0.85 Actual 0.57, LH Best Practices Expected 1 Actual 0.92
https://pr-2483.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.35, LH Best Practices Expected 1 Actual 0.92
https://pr-2483.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.49, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

jimbo

Looks good to me.

As our core workflows become more complicated in order to handle edges like this, let's keep an eye on it. The more types of products we support, the more strain it's going to put on talons as our only layer of abstraction; we may need to add another layer at some point.

sirugh

This seems functional but I wasn't sure about the initial state expectations. Lets talk about it.

sirugh · 2020-06-12T20:29:05Z

packages/peregrine/lib/talons/CartPage/ProductListing/EditModal/useEditModal.js

 import { useAppContext } from '../../../../context/app';

 export const useEditModal = () => {
    const [{ drawer }, { closeDrawer }] = useAppContext();
    const isOpen = drawer === 'product.edit';

+    const [variantPrice, setVariantPrice] = useState();


This is undefined initially. What happens if the item in the cart is a configurable item? I know it falls back to price but could that be wrong? Perhaps we need to do the calculation here.

The first render for this component will have item = undefined, so it wouldn't be that easy. I could add an effect, but I didn't really see the purpose when ProductDetail already had the data it needed to render price. Instead of having to handle a bunch of different states for opening, then resetting on close, I passed this responsibility down to ProductForm.

BTW - you can only edit Configurable product right now (assuming you meant isn't a configurable item)

Ah yes, I mistyped.

What I meant was that when you first click on a product to edit the value for variant was "undefined". It seemed to render correctly regardless, but maybe that was by coincidence?

By design. I'm willing to make this change, but did want to do some validation. If I add a product to my cart that is on sale, but don't checkout until a day later when that product is no longer on sale, do I still get that product at the sale price? I don't know the answer to that, but that would be a reason to use cart price instead of variant price. There might be other use cases as well where cart price is the accurate value, and is what we want to render.

If variant price reflects product specific sales I think it's fine to only use that. Would be odd to reflect cart sales such as BOGO or <$X discounts in an edit view.

If I add a product to my cart that is on sale, but don't checkout until a day later when that product is no longer on sale, do I still get that product at the sale price? I don't know the answer to that, but that would be a reason to use cart price instead of variant price.

We can assume that if a sale price expires before a shopper completes a purchase, the sale price is forfeit. If there are special cases where that's not true, someone would have to handle those cases as exceptions.

edit: Also, in general, we should use null rather than undefined as an initial value for useState().

sirugh · 2020-06-12T20:29:31Z

packages/venia-ui/lib/components/CartPage/ProductListing/EditModal/editModal.js

@@ -11,16 +11,20 @@ import ProductForm from './productForm';

 const EditModal = props => {
    const { item, setIsCartUpdating } = props;
-    const talonProps = useEditModal();
-    const { handleClose, isOpen } = talonProps;
+    const talonProps = useEditModal({ item });


I notice you pass the item here - is that to do the calculation of the variant price on initial render? You aren't using it anymore in the talon though.

I attempted your suggestion to initialize in the talon at first, but went the simple route. Will remove this prop.

sirugh · 2020-06-12T20:29:52Z

packages/venia-ui/lib/components/CartPage/ProductListing/EditModal/editModal.js


    const classes = mergeClasses(defaultClasses, props.classes);
    const rootClass = isOpen ? classes.root_open : classes.root;

    const bodyComponent = item ? (
        <div className={classes.body} key={item.id}>
-            <ProductDetail item={item} />
-            <ProductForm item={item} setIsCartUpdating={setIsCartUpdating} />
+            <ProductDetail item={item} variantPrice={variantPrice} />


variantPrice is undefined if you open the edit modal and don't click on anything.

Yes. We render cartItem price until a new variant is selected.

So if the item in the cart is a variant of the default, cartItem.price will be the selected variant price not the default price?

…-edit

dpatil-magento · 2020-06-17T18:54:04Z

Went ahead and QA'ed this, looks good.

tjwiebell added 2 commits June 10, 2020 15:38

Add price state that can be shared between static and dynamic compone…

45bb77e

…nts to show current selected price.

Remove obsolete files and updates tests

d14d988

m2-community-project bot added this to Ready for Review in Pull Request Progress Jun 10, 2020

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Jun 10, 2020

jimbo previously approved these changes Jun 11, 2020

View reviewed changes

m2-community-project bot assigned jimbo Jun 11, 2020

m2-community-project bot moved this from Ready for Review to Reviewer Approved in Pull Request Progress Jun 11, 2020

m2-community-project bot added the Progress: approved label Jun 11, 2020

sirugh requested changes Jun 12, 2020

View reviewed changes

m2-community-project bot assigned sirugh Jun 12, 2020

m2-community-project bot moved this from Reviewer Approved to Changes Requested in Pull Request Progress Jun 12, 2020

m2-community-project bot added Progress: changes requested and removed Progress: approved labels Jun 12, 2020

m2-community-project bot assigned tjwiebell Jun 12, 2020

m2-community-project bot moved this from Changes Requested to Review in Progress in Pull Request Progress Jun 12, 2020

m2-community-project bot added Progress: review and removed Progress: changes requested labels Jun 12, 2020

sirugh previously approved these changes Jun 15, 2020

View reviewed changes

tjwiebell added 2 commits June 16, 2020 10:13

Merge remote-tracking branch 'origin/develop' into tommy/configurable…

e5bf57f

…-edit

Remove unused prop and default variant price to null.

158865d

tjwiebell dismissed stale reviews from sirugh and jimbo via 158865d June 16, 2020 15:18

sirugh approved these changes Jun 16, 2020

View reviewed changes

Merge branch 'develop' into tommy/configurable-edit

3927f86

jimbo approved these changes Jun 18, 2020

View reviewed changes

dpatil-magento merged commit c3c7775 into develop Jun 18, 2020

dpatil-magento deleted the tommy/configurable-edit branch June 18, 2020 18:12

m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress Jun 18, 2020

m2-community-project bot removed the Progress: review label Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PWA-413] [Cart Page Edit] Price Does not get updated after changing the color/size. #2483

[PWA-413] [Cart Page Edit] Price Does not get updated after changing the color/size. #2483

tjwiebell commented Jun 10, 2020 •

edited

PWAStudioBot commented Jun 10, 2020 •

edited

devops-pwa-codebuild commented Jun 10, 2020 •

edited

jimbo left a comment

sirugh left a comment

sirugh Jun 12, 2020

tjwiebell Jun 12, 2020

sirugh Jun 13, 2020

tjwiebell Jun 15, 2020

sirugh Jun 15, 2020

jimbo Jun 15, 2020 •

edited

sirugh Jun 12, 2020

tjwiebell Jun 12, 2020

sirugh Jun 12, 2020

tjwiebell Jun 12, 2020

sirugh Jun 13, 2020

dpatil-magento commented Jun 17, 2020

[PWA-413] [Cart Page Edit] Price Does not get updated after changing the color/size. #2483

[PWA-413] [Cart Page Edit] Price Does not get updated after changing the color/size. #2483

Conversation

tjwiebell commented Jun 10, 2020 • edited

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jun 10, 2020 • edited

devops-pwa-codebuild commented Jun 10, 2020 • edited

jimbo left a comment

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbo Jun 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpatil-magento commented Jun 17, 2020

tjwiebell commented Jun 10, 2020 •

edited

PWAStudioBot commented Jun 10, 2020 •

edited

devops-pwa-codebuild commented Jun 10, 2020 •

edited

jimbo Jun 15, 2020 •

edited