ALFMOB-162: Improve Wishlist Functionality on PLP & Wishlist#42
ALFMOB-162: Improve Wishlist Functionality on PLP & Wishlist#42JoaoPinhoMinder wants to merge 4 commits intomainfrom
Conversation
37c5029 to
399378d
Compare
399378d to
b07538a
Compare
b07538a to
f5fc889
Compare
reisdev
left a comment
There was a problem hiding this comment.
Great additions! Just a couple comments for improvements 🤌🏼
|
|
||
| init(productId: String, product: Product?, dependencies: ProductDetailsDependencyContainer) { | ||
| var isAddToBagEnabled: Bool { | ||
| productHasStock |
There was a problem hiding this comment.
this could probably be inlined? 💅🏼
|
|
||
| if let baseProduct { | ||
| switch (product, selectedProduct) { | ||
| case (.some(let product), .none): |
There was a problem hiding this comment.
please avoid explicit pattern matching to Optional's .some / .none, and use let foo, let bar? or nil instead
| init( | ||
| productId: String, | ||
| product: Product?, | ||
| selectedProduct: SelectedProduct? = nil, |
There was a problem hiding this comment.
this API seems a bit overly complex, do we always need these 3 parameters, or could we have N different init overloads?
| return SelectedProduct(product: product, selectedVariant: selectedVariant) | ||
| } | ||
| } | ||
| } // swiftlint:disable:this file_length |
There was a problem hiding this comment.
we can add this at the top of the file via a single // swiftlint:disable file_length
however, would it be possible to modularize this logic so we keep file length under control? 🤓
| #endif | ||
|
|
||
| struct WishlistView<ViewModel: WishlistViewModelProtocol>: View { | ||
| @EnvironmentObject var coordinador: Coordinator |
There was a problem hiding this comment.
coordinador -> coordinator 😆
| @@ -0,0 +1,61 @@ | |||
| import Foundation | |||
|
|
|||
| public class SelectedProduct: Identifiable, Equatable, Hashable { | |||
There was a problem hiding this comment.
Hashable: Equatable, so the Equatable conformance is redundant
| public let product: Product | ||
| public let selectedVariant: Product.Variant | ||
|
|
||
| public var name: String { |
There was a problem hiding this comment.
please inline these computed properties for a more compact implementation 💄
(same thing for the other types)
|
|
||
| // MARK: Equatable | ||
|
|
||
| public static func == (lhs: SelectedProduct, rhs: SelectedProduct) -> Bool { |
There was a problem hiding this comment.
this is usually a bad™ idea (same thing for the custom Hashable conformance). Why do we need this?
(same thing for the other types)
There was a problem hiding this comment.
Since i had opened this PR in a wrong way, some discussions can also be threaten in this PR #41. Now i have changed the target branch to point to the branch of whoich this one depends on.
| @@ -0,0 +1,9 @@ | |||
| import Foundation | |||
|
|
|||
| public class BagProduct: SelectedProduct { | |||
There was a problem hiding this comment.
not a big fan of having models as classes... why do we need this? (same thing for the other Product types)
There was a problem hiding this comment.
I agree with you, did this only because for this use case, i needed to support in wishlist to have the "same" item multiple times if it is added in multiple color variants. Also, i require the Product.Variant to never have a size associated. So i end up with this strategy so i can override the id and also have specific initializations to omit size without polluting the VM with that specific logic.
But i'm open to ideas in this topic, i feel we are fighting the models we are receiving at the moment and exposed that situation which the solution was to mention that in the ticket 😅
| .foregroundStyle(Colors.primary.mono400) | ||
| Text.build(theme.font.paragraph.normal(configuration.selectedItem?.name ?? "")) | ||
| .foregroundStyle(Colors.primary.mono900) | ||
| Text.build(theme.font.paragraph.normal( |
There was a problem hiding this comment.
indentation is not right™, should probably be:
Text.build(
theme.font.paragraph.normal(
configuration.selectedItem?.name ?? configuration.noItemSelectedTitle
)
)
f5fc889 to
50f05b4
Compare
02e96df to
f69d424
Compare
46f9f58 to
17c4347
Compare
- Created `WishlistProduct` and `BagProduct` inheriting from `SelectedProduct`. - Made `WishlistProduct` unique by **product ID and color ID**. - Made `BagProduct` unique by **product ID and variant SKU**. - Removed size information from `WishlistView` and `ProductListingView`. - Updated **"Add to Bag"** action in `WishlistView` to redirect users to **PDP** for size selection instead of adding directly to the bag. - Disabled **"Add to Bag"** button on **PDP** until both **color** (pre-selected) and **size** have been selected.
ae00fcf to
18bc8b0
Compare
Fix rebases
Board Review — ALFMOB-162: Wishlist Improvements🔴 Cannot merge — Rewrite requiredThis PR (created March 2025) pre-dates the modular architecture refactor (ALFMOB-184, merged Dec 2025) and has 12 merge conflicts across critical files: Modify/Delete conflicts:
Content conflicts (10 files): BagView, BagViewModel, WishlistView, WishlistViewModel, ProductDetailsViewModel, ProductListingView, ProductListingViewModel, MockBagViewModel, BagViewModelProtocol, WishlistViewModelProtocol The PR targets the old monolithic structure ( Recommendation
|
Ticket
https://mindera.atlassian.net/browse/ALFMOB-162
Depends on: #41
Description
WishlistProductandBagProductinheriting fromSelectedProduct.WishlistProductunique by product ID and color ID.BagProductunique by product ID and variant SKU.WishlistViewandProductListingView.WishlistViewto redirect users to PDP for size selection instead of adding directly to the bag.Evidences
Screen.Recording.2025-03-12.at.09.40.43.mov