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

[Woo POS] Replace POS parent product with variable parent product #14818

Conversation

jaclync
Copy link
Contributor

@jaclync jaclync commented Jan 8, 2025

Part of #14702

Description

This is a followup of #14807 (comment):

What do you think about having a specific POSVariableParentProduct type, instead of using a type enum here? If we did that, attributes could be an internal property, and only used inside Yosemite. It might prove easier to handle than an enum with associated types, too.
It would mean having a different display approach for POSGroupedProduct or POSCategory, but it's reasonably likely that we'll want to display those differently in the grid anyway.

From my understanding, this suggestion is to replace POSItem enum parentProduct(POSParentProduct) case that previously had a type enum + associated type for each product type with a separate case per parent product type. This change also means that views that are meant to be general like the parent product card ParentProductCardView and its child items ChildItemList cannot depend on one parent product enum case. I originally wanted to make a view model for each view, but as we don't generally use view models in POS anymore, I added the view properties in the view initializer.

Key changes include:

Controller and Service Updates:

  • Updated PointOfSaleItemsController to handle POSVariableParentProduct instead of POSParentProduct in the loadInitialChildItems method and fetchVariationItems method. [1] [2] [3]

View Updates:

  • Modified ChildItemList and ItemList views to use POSVariableParentProduct and updated their initializers and property usage accordingly. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

Card View Updates:

  • Refactored ParentProductCardView to accept name, imageSource, and a detailView closure instead of POSParentProduct. This change simplifies the card view and makes it more flexible. [1] [2] [3]

Preview and Mock Updates:

  • Updated preview helpers and mock services to align with the new POSVariableParentProduct structure. [1] [2] [3] [4] [5]

Steps to reproduce

Testing information

🗒️ As the variable products support requires WC version 9.6+ which is not publicly released at the time of writing, I'd recommend commenting out L214 in ProductsRemote to include all product types.

  • Switch to a store eligible for POS and has at least one variable product
  • Go to Menu > Point of Sale mode
  • Tap on a variable product --> variations should be loaded shortly. each variation card should show the correct name with the attributes in the right order and include any attributes
  • Check a few other variable products and verify that the variation name is displayed correctly

Screenshots

Same as in #14807:


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

@jaclync jaclync added type: technical debt Represents or solves tech debt of the project. feature: POS labels Jan 8, 2025
@jaclync jaclync added this to the 21.4 milestone Jan 8, 2025
@wpmobilebot
Copy link
Collaborator

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14818-c2dd749
Version21.3
Bundle IDcom.automattic.alpha.woocommerce
Commitc2dd749
App Center BuildWooCommerce - Prototype Builds #12396
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@jaclync jaclync requested a review from joshheald January 8, 2025 06:03
@jaclync jaclync marked this pull request as ready for review January 8, 2025 06:03
Copy link
Contributor

@joshheald joshheald 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! One suggestion in the view inits, but what you have is fine too.

Comment on lines +18 to +20
init(parentItem: POSItem, title: String) {
self.parentItem = parentItem
self.parentProduct = parentProduct
self.title = title
Copy link
Contributor

Choose a reason for hiding this comment

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

I much prefer this simplification – when there's so little to pass through, the view's more flexible if we just declare the title compared to passing a whole model object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

Comment on lines +32 to +38
ParentProductCardView(name: parentProduct.name,
imageSource: parentProduct.productImageSource,
detailView: {
Text(Localization.variationsAvailable)
.foregroundStyle(Color.posSecondaryText)
.font(.posBodyRegular)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

It would also be fine to make this a VariableParentProductCardView, and continue to pass through the whole parentProduct here. Since we have to define the detail view as a closure, it's not quite as nice as the ChildItemList view above, where we simply pass through title string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The image + title design seems pretty fixed for parent product types, I think it's nice sharing most of the view except the detail view. However, if the future design has more differences than the detail view, let's move to a separate view per parent product case for sure.

struct ParentProductCardView<DetailView: View>: View {
private let name: String
private let imageSource: String?
private let detailView: DetailView
Copy link
Contributor

Choose a reason for hiding this comment

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

If you keep this – I think you can make this a ViewBuilder, so you only call it when it's used not at init time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for finding the best practice, TIL

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 21.4. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@jaclync jaclync merged commit 5211f5d into trunk Jan 9, 2025
26 of 28 checks passed
@jaclync jaclync deleted the feat/14702-replace-POSParentProduct-with-POSVariableParentProduct branch January 9, 2025 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: POS type: technical debt Represents or solves tech debt of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants