Skip to content

Conversation

iamgabrielma
Copy link
Contributor

Closes: #

Description

Steps to reproduce

Testing information

Screenshots


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

@dangermattic
Copy link
Collaborator

dangermattic commented Jun 27, 2024

2 Warnings
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 27, 2024

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 Numberpr13179-a28f8e6
Version19.2
Bundle IDcom.automattic.alpha.woocommerce
Commita28f8e6
App Center BuildWooCommerce - Prototype Builds #9769
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.


struct CartView: View {
@ObservedObject private var viewModel: PointOfSaleDashboardViewModel
@ObservedObject private var dashboardViewModel: PointOfSaleDashboardViewModel
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 reference to the dashboard is currently necessary due to .submitCart(). This calls for syncOrder(for cartProducts: [CartItem]) async which contains OrderService and card reader logic

addMoreButton
.padding(32)
.disabled(viewModel.isAddMoreDisabled)
.disabled(dashboardViewModel.isAddMoreDisabled)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This .disabled state relies on the payment state, possibly shouldn't be attached to the cart logic, but totals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to move forward with this PR @iamgabrielma?

I noticed it involves isAddMoreDisabled which I've updated in my PR to also take into account order syncing state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iamgabrielma iamgabrielma added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Jun 27, 2024
@iamgabrielma iamgabrielma changed the title [Woo POS] Extract cart logic to CartViewModel [Woo POS] Extract CartViewModel and TotalsViewModel Jun 27, 2024
@iamgabrielma iamgabrielma deleted the task/12998-extract-cart-logic-to-vm branch October 8, 2024 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: do not merge Dependent on another PR, ready for review but not ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants