Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Adding to Wishlist from the Product List Page #2

Merged
merged 37 commits into from
Aug 24, 2021

Conversation

alexvuong
Copy link
Collaborator

@alexvuong alexvuong commented Aug 9, 2021

JIRA:

How to test-drive this PR

  • Spin up the app
  • Nvaigate to PLP
  • Click on heart icon on the product tile
  • Login if prompted
  • You should see a toast - "1 item added to wishlist"
  • Navigate to wishlist page in Account Management to verify item was added to wishlist
  • Navigate back to PLP
  • Click on solid heart icon to remove item from wishlist
  • Confirm by clicking "Yes, remove item" when promoted
  • Verify, heart icon toggles to outline

Changes

  • Added wishlist icon on product item on PLP

Todos

General

  • Add a high-level description of your changes to relevant CHANGELOG.md files (not required for doc updates)
  • The code changes are covered by test cases
  • Analytics events instrumented to measure impact of change

Backwards Compatibility

  • This PR contains no breaking changes or breaking changes have been approved by Product Management/Engineering as necessary.

Breaking changes include:

  • Removing a public function/Component/Prop
  • Adding required arguments to functions
  • Changing the data-types of function params or return values

Accessibility Compliance

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

  • There are no changes to UI

or...

Documentation

Documentation and comments should follow the Mobify Content Style Guide and Mobify's Capitalization and Spelling List

@alexvuong alexvuong self-assigned this Aug 9, 2021
@bendvc bendvc changed the title Add to wishlist PLP Allow Adding to Wishlist from the Product List Page Aug 11, 2021
@alexvuong alexvuong requested a review from adamraya August 11, 2021 20:29
Copy link
Collaborator

@adamraya adamraya left a comment

Choose a reason for hiding this comment

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

Loading the PLP shows a warning in the browser console:

Warning: Failed prop type: The prop `onOpen` is marked as required in `ConfirmationModal`, but its value is `undefined`.

Copy link
Collaborator

@adamraya adamraya left a comment

Choose a reason for hiding this comment

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

Wishlist product quantity not being updated after adding a product that already existed on the wishlist.

If the shopper has a product already added to the wishlist, the shopper navigates as a Guest shopper and adds the same product. After going through the login flow, the notification "1 item added to wishlist" is displayed, but the quantity of the product in the wishlist is not updated.

(Same happens when the shopper clicks the PDP "Add to wishlist button" of a product already added to the Wishlist)

@alexvuong
Copy link
Collaborator Author

Wishlist product quantity not being updated after adding a product that already existed on the wishlist.

If the shopper has a product already added to the wishlist, the shopper navigates as a Guest shopper and adds the same product. After going through the login flow, the notification "1 item added to wishlist" is displayed, but the quantity of the product in the wishlist is not updated.

(Same happens when the shopper clicks the PDP "Add to wishlist button" of a product already added to the Wishlist)

Hey @adamraya, Thanks for the feedback. We are currently allowing adding the same item to wishlist, this will make the quantity increase for that item, which can be seen in the wishlist page. I will have a look at the wishlist page to make sure the number is shown correctly

@jennmills
Copy link
Contributor

jennmills commented Aug 12, 2021

UX/UI Review

  • Remove confirmation shouldn't appear on PLP. See recording
  • Change title of Remove confirmation modal to "Remove item?" See screenshot
  • Only show one toast at a time to reduce cognitive load. See recording => we've agreed to allow multiple toast for multiple actions
  • Titles of products should be clickable and take a shopper to the PDP. See recording

Copy link
Collaborator

@adamraya adamraya left a comment

Choose a reason for hiding this comment

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

  • Error when the user tries to add products to the Wishlist from the PDP:
{
  "title": "Product List Product Id Missing",
  "type": "https://api.commercecloud.salesforce.com/documentation/error/v1/errors/product-list-product-id-missing",
  "detail": "The product ID is missing."
}

Steps to reproduce:

  1. Navigate to any PDP. e.g. http://localhost:3000/en/product/25518837M?color=JJ908XX&size=9LG&pid=701642873334M
  2. Choose the variant of the product.
  3. Click the "Add to wishlist" button.

  • Adding multiple products to the Wishlist from the PLP shows a warning in the browser console:
Warning: Encountered two children with the same key, `something went wrong. try again!-error`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.

  • Clicking quickly (before the last Toast disappers) the heart icon on the product tile in the PLP to add/remove the product from the Wishlist shows Toasts not disappearing:
    Screen_Shot_2021-08-19_at_4_51_22_PM

The Wishlist page http://localhost:3000/en/account/wishlist doesn't load when the Wishlist has more than 25 items, showing the network error:

{
  "title": "Bad Request",
  "type": "https://api.commercecloud.salesforce.com/documentation/error/v1/errors/validation",
  "detail": "Maximum number of products you can request in one call is 25."
}

@@ -33,7 +36,7 @@ export function useToast() {
variant = 'subtle',
isClosable = true
}) => {
const toastId = `${title}-${status}`.toLowerCase()
const toastId = `${title}-${status}-${id}`.toLowerCase()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The id has always the value undefined when clicking the "Add to wishlist" button on the PDP.

@alexvuong alexvuong requested a review from a team as a code owner August 23, 2021 17:20
* Callback function to be invoked when the user removes item to wishlist
*/
onRemoveWishlistClick: PropTypes.func,
isWishlistLoading: PropTypes.func
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small warning: react_devtools_backend.js:2842 Warning: Failed prop type: Invalid prop isWishlistLoading of type boolean supplied to ProductTile, expected function.

Copy link
Collaborator

@adamraya adamraya left a comment

Choose a reason for hiding this comment

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

Looks good @alexvuong, thanks for dealing with those async Toast events!

@alexvuong alexvuong merged commit 78cd86f into develop Aug 24, 2021
@kevinxh
Copy link
Collaborator

kevinxh commented Aug 24, 2021

@alexvuong @adamraya @bendvc I'm surprised how much complexity we added to individual pages just to enable wishlist, I'd imagine it will be very very hard for developers to ramp up and make changes on these wishlist related code, as a result, devs will have a very high chance of just ripping all the code and start fresh, as Mobify we've made that mistake in the past. We should think about modularize the code, or making architecture/ pattern changes to avoid having to add hundreds of lines of code to individual pages (PDP, PLP, cart, etc...). :( IMO, it is really important to keep those main pages clean and simple.

Screen Shot 2021-08-24 at 9 55 21 AM

(screenshot of example of how many lines of unorganized code we added to PLP)

Again, wishlist related logic should be encapsulated in a single module like useCusomterProductList hook.

@bredmond-sf
Copy link
Collaborator

making architecture/ pattern changes

During the story I worked on, the duplicate code path because of sometimes using the queue and sometimes not stuck out to me. I think the biggest, quickest win would be to talk through either eliminating it or I think more likely, always using it.

@kevinxh kevinxh mentioned this pull request Sep 1, 2021
9 tasks
@alexvuong alexvuong deleted the feature-add-to-wishlist-plp-new branch March 8, 2022 19:08
bendvc added a commit that referenced this pull request Jan 14, 2025
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.

5 participants