-
Notifications
You must be signed in to change notification settings - Fork 143
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
Recommended Products: fix toggling of the favorite icon (@W-16093853@) #1861
Conversation
onFavouriteToggle: (toBeFavourite) => { | ||
const action = toBeFavourite ? addItemToWishlist : removeItemFromWishlist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By renaming the parameter, I think this should be clearer that it's the target favourite state (not the current state). So we don't get confused whether to add or remove the item.
onFavouriteToggle: (toBeFavourite) => { | ||
const action = toBeFavourite ? addItemToWishlist : removeItemFromWishlist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add a test yet this time. For this case, we would have to create a test file from scratch because RecommendedProducts do not have one yet.
Since I wanted to test the favourite toggle only, I'd have to mock a bunch of things just to get to the favourite-toggle part of the code. I don't think this is going to be a good test file.
I think a better approach is to do some refactoring (and then test the new code). There are many places with very similar code and they all add/remove item to/from the wishlist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've verified that we can now add to wishlist from the recommended products section
This PR fixes a bug with the
RecommendedProducts
such that clicking on the heart icon would toggle the favourite state successfully. It was not doing anything at all previously.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