ProductFullDetail talon #1814

sirugh · 2019-10-01T14:32:28Z

NOTE:

This PR refactors useCarousel to have a local state for activeIndex instead of using a reducer. This is because of a bug found in using the hook with a variable length input array of images.

This PR also changes the PDP to show the correct amount of images when the PDP first renders and to show the correct image after a color is selected.

packages/peregrine/lib/talons/ProductFullDetail/useProductFullDetail.js

sirugh · 2019-10-01T14:39:08Z

packages/venia-ui/lib/components/ProductFullDetail/productFullDetail.js

                    />
                </p>
            </section>
            <section className={classes.imageCarousel}>
-                <Carousel
-                    images={mediaGalleryEntries.value}
-                    key={mediaGalleryEntries.key}


key didn't seem used so I deleted the code generating it and just returned the computed entries.

Just from looking quickly, I think it's safe to say there was a valid reason to calculate that key before. Did something change so that it's no longer necessary?

sirugh · 2019-10-01T14:40:22Z

packages/venia-ui/lib/components/ProductOptions/options.js

@@ -24,6 +24,8 @@ class Options extends Component {
        const { handleSelectionChange, props } = this;
        const { product, selectedValues = [] } = props;

+        // TODO: Do this check in parent and only pass `configurable_options`
+        // instead of the entire `product` as a prop.


Fun note, we already do this in the mini cart product options page but we don't do it in the PFD. 🤷

PWAStudioBot · 2019-10-01T15:48:38Z

	Fails
🚫	Missing "Description" section. Please add it back, with detail.
🚫	Missing "Verification Steps" section. Please add it back, with detail.
🚫	No linked issue found. Please link a relevant open issue by adding the text "closes #<issue_number>" or including the associated JIRA ID in your PR.

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

If your PR is missing information, check against the original template here. At a minimum you must have the section headers from the template and provide some information in each section.

Generated by 🚫 dangerJS against 494395d

…fulldetail

sirugh · 2019-10-03T15:48:15Z

packages/peregrine/lib/talons/ProductFullDetail/useProductFullDetail.js

+    // Selections are initialized to "code => undefined". Once we select a value, like color, the selections change. This filters out unselected options.
+    const optionsSelected =
+        Array.from(optionSelections.values()).filter(value => !!value).length >
+        0;


This is new code.

dpatil-magento · 2019-10-04T04:15:33Z

QA Pass, good to merge once review is approved.

jimbo · 2019-10-07T15:12:25Z

packages/peregrine/lib/hooks/useCarousel.js

+        } else {
+            setActiveItemIndex(images.length - 1);
+        }
+    }, [activeItemIndex, images]);


When using useCallback, look out for scenarios like this one. As written, handlePrevious will be recreated whenever activeItemIndex changes, and since handlePrevious itself changes activeItemIndex, this memoization is pointless.

Before, we were keeping index in a reducer so that handlePrevious could be memoized. If you're going to remove the reducer in favor of useState, you'll want to use the functional update form.

const handlePrevious = useCallback(() => { setActiveItemIndex(prev => prev > 0 ? prev - 1 : images.length - 1) }, [images])

jimbo · 2019-10-07T15:21:21Z

packages/peregrine/lib/hooks/useCarousel.js

-    );
+    const handleNext = useCallback(() => {
+        setActiveItemIndex((activeItemIndex + 1) % images.length);
+    }, [activeItemIndex, images]);


This memoization also fails, for the same reason.

const handleNext = useCallback(() => { setActiveItemIndex(prev => (prev + 1) % images.length) }, [images])

jimbo · 2019-10-07T15:43:14Z

packages/venia-ui/lib/components/ProductFullDetail/productFullDetail.js

                    />
                </p>
            </section>
            <section className={classes.imageCarousel}>
-                <Carousel
-                    images={mediaGalleryEntries.value}
-                    key={mediaGalleryEntries.key}


Just from looking quickly, I think it's safe to say there was a valid reason to calculate that key before. Did something change so that it's no longer necessary?

jimbo · 2019-10-07T15:43:53Z

packages/venia-ui/lib/components/ProductFullDetail/productFullDetail.js

                <p className={classes.productPrice}>
                    <Price
-                        currencyCode={productPrice.currency}
-                        value={productPrice.value}
+                        currencyCode={productDetails.price.currency}


Is there any way this could result in an error being thrown?

https://github.com/magento/pwa-studio/pull/1814/files#diff-dd082926e212e0be673b35eb01025b3aL101

productPrice was previously just amount which is an object containing currency and value props. The normalization done in the talon is equivalent with only a rename of productPrice to productDetails.price.

Technically the old way (and the new method) can both throw if any of the values are undefined in the chain. Since we don't yet have option chaining I can leave these to some default.

dpatil-magento · 2019-10-07T18:03:43Z

@sirugh If user select fashion color and size and then clicks/loads 2nd or 3rd image and then click on Add to Cart, automatically first image gets loaded. On develop branch it stays on selected images only.

…ry component was triggering re-renders/remaps of thus triggering updates down the ProductFullDetail tree

dpatil-magento · 2019-10-07T21:35:32Z

@sirugh Simple Product page does not load anymore https://pr-1814.pwa-venia-staging.com/carmina-earrings.html :(

dpatil-magento · 2019-10-08T20:09:53Z

QA Pass, Need reviewer approval to get merged.

ProductFullDetail talon

d0a632d

sirugh commented Oct 1, 2019

View reviewed changes

packages/peregrine/lib/talons/ProductFullDetail/useProductFullDetail.js Outdated Show resolved Hide resolved

m2-community-project bot assigned sirugh Oct 1, 2019

m2-community-project bot added the Progress: review label Oct 1, 2019

sirugh commented Oct 1, 2019

View reviewed changes

memoizes some things

0498482

PWAStudioBot added the pkg:peregrine label Oct 1, 2019

PWAStudioBot added the pkg:venia-ui label Oct 1, 2019

m2-community-project bot removed the Progress: review label Oct 1, 2019

supernova-at unassigned sirugh Oct 1, 2019

Merge branch 'develop' into rugh/talon-productfulldetail

797f46c

sirugh added version: Patch This changeset includes backwards compatible bug fixes. hold On hold until another condition is fulfilled. labels Oct 1, 2019

Merge remote-tracking branch 'origin/develop' into rugh/talon-product…

7c32202

…fulldetail

sirugh removed the hold On hold until another condition is fulfilled. label Oct 1, 2019

jimbo previously approved these changes Oct 1, 2019

View reviewed changes

m2-community-project bot assigned jimbo Oct 1, 2019

m2-community-project bot added the Progress: approved label Oct 1, 2019

sirugh mentioned this pull request Oct 1, 2019

Product Options Talons #1819

Merged

m2-community-project bot removed the Progress: approved label Oct 1, 2019

Merge branch 'develop' into rugh/talon-productfulldetail

1a78997

m2-community-project bot added the Progress: changes requested label Oct 2, 2019

sirugh commented Oct 3, 2019

View reviewed changes

sirugh and others added 5 commits October 3, 2019 11:10

Merge branch 'develop' into rugh/talon-productfulldetail

ee1d539

fix test

e134bfa

Merge branch 'develop' into rugh/talon-productfulldetail

36fe623

Add comment explaining media gallery image selection

91e253d

number of selected options check is now accurate

05542ad

jimbo suggested changes Oct 7, 2019

View reviewed changes

m2-community-project bot added Progress: changes requested and removed Progress: review labels Oct 7, 2019

Fix callbacks to use functional state setting

ce4c010

jimbo previously approved these changes Oct 7, 2019

View reviewed changes

Merge branch 'develop' into rugh/talon-productfulldetail

2b83b2c

m2-community-project bot added Progress: approved and removed Progress: changes requested labels Oct 7, 2019

m2-community-project bot removed the Progress: approved label Oct 7, 2019

Refactor product to function component since something in the old Que…

765fe9d

…ry component was triggering re-renders/remaps of thus triggering updates down the ProductFullDetail tree

sirugh dismissed jimbo’s stale review via 765fe9d October 7, 2019 19:53

Add a comment

4309173

Merge branch 'develop' into rugh/talon-productfulldetail

494395d

jimbo approved these changes Oct 8, 2019

View reviewed changes

jimbo merged commit 1b79b5c into develop Oct 8, 2019

dpatil-magento deleted the rugh/talon-productfulldetail branch October 8, 2019 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProductFullDetail talon #1814

ProductFullDetail talon #1814

sirugh commented Oct 1, 2019 •

edited

Loading

sirugh Oct 1, 2019

jimbo Oct 7, 2019

sirugh Oct 1, 2019

PWAStudioBot commented Oct 1, 2019 •

edited

Loading

sirugh Oct 3, 2019

dpatil-magento commented Oct 4, 2019

jimbo Oct 7, 2019

jimbo Oct 7, 2019

jimbo Oct 7, 2019

jimbo Oct 7, 2019

sirugh Oct 7, 2019

dpatil-magento commented Oct 7, 2019 •

edited

Loading

dpatil-magento commented Oct 7, 2019

dpatil-magento commented Oct 8, 2019

ProductFullDetail talon #1814

ProductFullDetail talon #1814

Conversation

sirugh commented Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PWAStudioBot commented Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

dpatil-magento commented Oct 4, 2019

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 Oct 7, 2019 • edited Loading

dpatil-magento commented Oct 7, 2019

dpatil-magento commented Oct 8, 2019

sirugh commented Oct 1, 2019 •

edited

Loading

PWAStudioBot commented Oct 1, 2019 •

edited

Loading

dpatil-magento commented Oct 7, 2019 •

edited

Loading