Skip to content

Conversation

@kevinxh
Copy link
Contributor

@kevinxh kevinxh commented Sep 29, 2021

Description

This PR allows adding wishlist items to the cart.

This PR was suppose to be a small change but turns out needing to refactor a few components include AddToCartModal and ProductView. The reason is the AddToCartModal was embedded in ProductView as a local component. This works okay until we need to have ProductView in a modal. In the below screen, you will see a situation where we need to display the AddToCartModal after successfully adding item to the cart from the ProductView modal.

Screen Shot 2021-10-05 at 3 53 48 PM

Because the component hierarchy looks like this:

<ProductViewModal>
  <ProductView>
    <AddToCartModal />
  </ProductView>
</ProductViewModal>

This is limiting the ability to display the two modals separately. We need to support the situation where AddToCartModal is rendered on the page while ProductViewModal is not.

The implementation becomes really messy after i'm trying to add a new AddToCartModal in the wishlist components.

So, I decided to move AddToCartModal to top level as a global component, as this modal is shown on multiple pages like pdp, cart and wishlist. All place share the same modal. To achieve that, i also need to create a context to store the modal open/close state.

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

  • Move addtocartmodal out of ProductView

How to Test-Drive This PR

  • npm start
  • Go to http://localhost:3000
  • Add a few items to your wishlist, include both master product and variant products
  • Then go to http://localhost:3000/en-GB/account/wishlist , click add to cart on your variant item
  • verify the item has been successfully added to cart
  • click "select option" on a master item, and use the modal to add item to the cart, verify you can do so successfully.

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

Localization

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

@kevinxh kevinxh requested a review from a team as a code owner September 29, 2021 23:41
@kevinxh kevinxh requested a review from alexvuong September 30, 2021 20:33
@kevinxh kevinxh changed the title Fix wishlist item add to cart Add wishlist item to the cart Oct 7, 2021
parameters: {
ids: ids.join(',')
ids: ids.join(','),
allImages: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need this for the swatch images.

@alexvuong
Copy link
Contributor

Some regression that I found on the add to cart modal

  • The add to cart is still open when I transition from PDP to cart page after I add an item to cart. In general, It seems like the modal won't close when we transition from one page to another
    image
  • The modal is too low when there is the recommended section
    image

@alexvuong
Copy link
Contributor

I also notice a pattern for the modal components. We tend to keep the component in the same place we create the hook. Eg. the login modal and add to cart modal. We might want to do the same for the productViewModal for consistency here.

This will not require a change on this PR, but it can potentially create another refactor for the productViewModal

import {findImageGroupBy} from '../utils/image-groups-utils'

export const AddToCartModalContext = React.createContext()
export const useAddToCartModalContext = () => useContext(AddToCartModalContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we declare this context inside the index.js under the context folder? How should we decide which context will stay in its own file or inside the context file?

Copy link
Contributor Author

@kevinxh kevinxh Oct 8, 2021

Choose a reason for hiding this comment

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

Should we declare this context inside the index.js under the context folder?

I'd say no, because i think the AddToCartModalContext itself doesn't work alone, it's not made to be shared with other modules or features. I view it as a part of the "Add to cart modal" module. This file encapsulate everything to make add to cart modal work in the app.

How should we decide which context will stay in its own file or inside the context file?

So i think this question comes down to recognizing the separation of concerns. A mental model for myself is like OO programming's principle Encapsulation, I view the module (e.g. this use-add-to-cart-modal.js file) as a "class" and everything related to this module should be encapsulated in the file. I think this pattern helps developers avoiding "Spagetti code" (generally because of bad separation of concerns)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed develop branch has a bunch of changes made to context.js (the update category context and the new currency context). Seems like we already established a pattern there, i guess i'll follow the pattern and maybe we can chat a bit more in tech discussion session.

TL;DR i'm moving this to context.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't do this because there is a circular dependency between the context, hook and the component... I really don't want to separate the component from the hook and contexts because the component itself doesn't work, it is meant to work only with the hook.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding a comment to explain that so it won't confuse other developers?

Copy link
Contributor

@alexvuong alexvuong left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinxh kevinxh merged commit 915bf3e into develop Oct 13, 2021
@kevinxh kevinxh deleted the W-9284634_add-wishlist-to-cart branch October 13, 2021 16:29
vmarta added a commit that referenced this pull request Oct 14, 2021
commit 915bf3e
Author: Kevin He <[email protected]>
Date:   Wed Oct 13 09:29:43 2021 -0700

    Add wishlist item to the cart (#158)

    * fix variant id

    * include ProductViewModal in wishlist item

    * refactor addtocartmodal to global

    * refactor AddToCartModal

    * remove modal from wishlist action

    * fix missing swatch images

    * add changelog

    * fix tests

    * close modal when path change

    * add AddToCartModalProvider

    * remove useMemo

    * fix lint

    * optimize modal rerenders

    * fix test

    * remove unused import

commit 19cc8d4
Author: vcua-mobify <[email protected]>
Date:   Tue Oct 12 13:37:08 2021 -0700

    Add a cache-control header to the PDP (#172)

    * Add a cache-control header to the PDP

    Co-authored-by: Ben Chypak <[email protected]>

commit 2810522
Author: svc-scm <[email protected]>
Date:   Tue Oct 12 05:57:59 2021 -0700

    Updated/Added CODEOWNERS with ECCN

commit a2372d9
Author: Ben Chypak <[email protected]>
Date:   Fri Oct 8 16:47:20 2021 -0700

    Invert equality comparison (#174)

commit a329fa8
Author: Kevin He <[email protected]>
Date:   Fri Oct 8 13:53:46 2021 -0700

    always enable the fav icon in product tiles (#173)

commit 62d55bb
Author: vcua-mobify <[email protected]>
Date:   Thu Oct 7 17:56:55 2021 -0700

    Update localization cli and readme (#160)

    * Update localization cli and readme

    * Fix heading hierarchy

    * Update packages/pwa/app/translations/README.md

    Applying suggested change to the readme

    Co-authored-by: Vincent Marta <[email protected]>

    * Update packages/pwa/app/translations/README.md

    Applying suggested change to the readme

    Co-authored-by: Vincent Marta <[email protected]>

    * Clean up references to old CLI and update README

    * Update README.md

    * Update readme to remove references to locale.config.js

    * Update packages/pwa/app/translations/README.md

    Co-authored-by: Vincent Marta <[email protected]>

    * Update packages/pwa/app/translations/README.md

    Co-authored-by: Vincent Marta <[email protected]>

    * Update packages/pwa/app/translations/README.md

    Co-authored-by: Vincent Marta <[email protected]>

    Co-authored-by: Vincent Marta <[email protected]>

commit b0db12a
Author: John Boxall <[email protected]>
Date:   Thu Oct 7 11:32:56 2021 -0700

    Remove the `X-Powered-By` HTTP response header
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants