Skip to content

[Woo POS] Expose item controllers for POS list views to use directly #15504

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

Merged
merged 7 commits into from
Apr 14, 2025

Conversation

joshheald
Copy link
Contributor

@joshheald joshheald commented Apr 11, 2025

Closes: #

Description

This PR exposes the items controllers, and then switches between them in the ItemListView.

ItemList and ChildItemList (mostly) don't know what controller they're using, they have it passed in by the ItemListView as we swap between them.

Initially, in 452247f I did the switching in the dashboard view and then had one ItemListView per controller... but I actually think this is better. When we have searching for multiple item types, we might want to change the responsibilities of the ItemListView or add another intermediate view, to simplify knowing which controller we're searching... but equally, it might work fairly neatly.

Steps to reproduce

Enable the coupons and search feature flags

  1. Launch the app and navigate to POS
  2. Use the items list, search, and coupons to build and edit a cart
  3. Add a coupon to the store
  4. Refresh, and simulate errors/recovery
  5. Observe that the lists work as expected

N.B. the search list is far from perfect, but it's all behind a feature flag. I'll improve it further in future PRs

Testing information

Tested on iPad Air running iOS 17.7.3, and a simulator running 18.3

Screenshots


  • 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.

@joshheald joshheald added type: task An internally driven task. status: do not merge Dependent on another PR, ready for review but not ready for merge. feature: POS labels Apr 11, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Apr 11, 2025

1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Apr 11, 2025

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

App NameWooCommerce iOS Prototype
Build Number29297
VersionPR #15504
Bundle IDcom.automattic.alpha.woocommerce
Commit6e27621
Installation URL4k6fgi6s79jb0
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@@ -14,21 +14,23 @@ struct ItemListView: View {

@Binding var selectedItemType: ItemType

var itemListController: PointOfSaleItemsControllerProtocol {
Copy link
Contributor

@staskus staskus Apr 11, 2025

Choose a reason for hiding this comment

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

Let's call selected ones selected or current, or it's easy to conflict with actual itemsController

Suggested change
var itemListController: PointOfSaleItemsControllerProtocol {
var selectedItemListController: PointOfSaleItemsControllerProtocol {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's probably clear enough now that the posModel only has specifically named controllers, no general itemsController – WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes 👍 then it's clear.

@joshheald joshheald marked this pull request as ready for review April 11, 2025 14:38
@joshheald joshheald added this to the 22.2 milestone Apr 11, 2025
@joshheald joshheald removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Apr 11, 2025
@joshheald joshheald changed the title Woomob 149 expose item controllers [Woo POS] Expose item controllers for POS list views to use directly Apr 11, 2025
@staskus staskus self-requested a review April 14, 2025 07:06
Base automatically changed from task/update-sentry to trunk April 14, 2025 11:09
Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

Thank you for this refactoring. Looks good. Thanks for additional improvements of extracting VM out of the view.

Tested on iOS 18 iPad simulator, tested products and variations with features flags disabled, and additionally coupons with feature flags enabled.

@joshheald joshheald merged commit 03aca1c into trunk Apr 14, 2025
16 of 18 checks passed
@joshheald joshheald deleted the woomob-149-expose-item-controllers branch April 14, 2025 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: POS type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants