[Woo POS] Split container and list states with new pagination #14727

joshheald · 2024-12-18T16:39:28Z

Closes: #14693

Description

This updates the Items view state to split out container states from list states.

It also lays the groundwork for having a hierarchy of lists including variation details.

Testing information

This changes how the state is represented for the item list as a container.

Testing should cover:

initial loading (full screen)
Empty state (full screen)
Error state (full screen – with retry)
Using items in the cart
Scrolling down to load the next page
Pull to refresh

Errors during 2nd and subsequent page loads and pull to refresh will still be full screen, as will empty state.

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.

dangermattic · 2024-12-18T16:40:05Z

	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 · 2024-12-18T16:50:50Z

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

	App Name	WooCommerce iOS
	Build Number	pr14727-00fc51f
	Version	21.2
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`00fc51f`
	App Center Build	WooCommerce - Prototype Builds #12237

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

jaclync

Apology for the overlapping changes that took longer than I expected to wrap up, I foresee some conflicts in the items controller. Left some initial comments, will review it in depth tomorrow.

WooCommerce/Classes/POS/Models/ItemsStackState.swift

Networking/Networking/Remote/ProductsRemote.swift

jaclync · 2024-12-19T14:24:05Z

WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift

-    private var isInitialLoading: Bool = true
-    private let paginationTracker: PaginationTracker
+    private(set) var itemsViewStatePublisher: any Publisher<ItemsViewState, Never>
+    private var itemsViewStateSubject: PassthroughSubject<ItemsViewState, Never> = .init()


Any reasons not having this as a CurrentValueSubject and use its value when needed, or a @Published var?

Not a published var because it's not an observable object (and I wanted to keep POSModel as the only one, partly for possibly easier moves to Observable.

It could be a CurrentValueSubject, I just haven't tried it. No time left today but feel free to make that change if you think it'd be an improvement...

I see, I think it's an improvement not having to need a separate private variable with didSet to sync with the subject. Updated in 00fc51f and it seems to work fine from testing.

…3-split-container-and-list-states-with-new-pagination

joshheald · 2024-12-19T17:44:21Z

@jaclync this has the pagination changes merged in now – testing since then has been limited.

There's definitely still an issue when the page size is small, on pull to refresh – the pull to refresh view translation seems to trigger the loadNextItems as well, so sometimes all the items get cleared out and you see the skeleton view.

That gesture needs a work anyway, because of the pre-existing issues, so I'm treating it as out of scope for this PR.

Let me know what you think about the approach

… version `AsyncPaginationTracker`.

jaclync

Looks good so far with testing, I confirm the issue where pulling to refresh without a full height of items triggers the network request twice even with page size 100. Created an issue #14735.

Updating the PR from the comments, and will test again.

WooCommerce/Classes/POS/Presentation/ItemListView.swift

jaclync · 2024-12-20T01:12:05Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift


            POSConnectivityView()
        }
        .environment(\.floatingControlAreaSize,
                      CGSizeMake(floatingSize.width + Constants.floatingControlHorizontalOffset,
                                 floatingSize.height + Constants.floatingControlVerticalOffset))
        .environment(\.posBackgroundAppearance, posModel.paymentState != .processingPayment ? .primary : .secondary)
-        .animation(.easeInOut, value: posModel.itemListState == .initialLoading)
+        .animation(.easeInOut, value: posModel.itemsViewState.containerState == .loading)


Nice having a separate container state from the changes in this file 👍

WooCommerce/Classes/POS/Utils/PreviewHelpers.swift

WooCommerce/Classes/Tools/InfiniteScroll/PaginationTracker.swift

WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleItemsControllerTests.swift

…ueSubject` so that it does not require another private variable with didSet.

jaclync

LGTM from testing with WPCOM account login and site credentials login, and the scenarios in the testing steps Just the issue when the item list is short on pull-to-refresh, which I will look into as a separate fix.

joshheald · 2024-12-20T08:01:22Z

LGTM from testing with WPCOM account login and site credentials login, and the scenarios in the testing steps Just the issue when the item list is short on pull-to-refresh, which I will look into as a separate fix.

Thanks for all the improvements @jaclync!

joshheald added 4 commits December 18, 2024 16:00

14693 Separate container states from list states

6bdb9f3

14693 Update tests for split container/list states

626087e

Fix tests

6e661f2

Revert POS product page size to 100

557b450

joshheald requested a review from jaclync December 18, 2024 16:39

joshheald added type: task An internally driven task. feature: POS labels Dec 18, 2024

joshheald added this to the 21.3 milestone Dec 18, 2024

joshheald added 3 commits December 18, 2024 17:00

14693 maintain async flow with paginationtracker

ef29bad

14693 Tidy up state usage

efc038f

14693 Use a struct for the stack state

47f3e10

jaclync reviewed Dec 19, 2024

View reviewed changes

Base automatically changed from feat/14699-pagination-headers-products to trunk December 19, 2024 16:12

joshheald added 2 commits December 19, 2024 17:34

Merge branch 'feat/14699-pagination-headers-products' into issue/1469…

ac570eb

…3-split-container-and-list-states-with-new-pagination

14693 code review fixes

ab6223b

joshheald marked this pull request as ready for review December 19, 2024 17:44

joshheald modified the milestones: 21.3, 21.4 Dec 19, 2024

jaclync self-assigned this Dec 20, 2024

Revert changes in PaginationTracker now that POS is using the async…

1d3ffd2

… version `AsyncPaginationTracker`.

jaclync approved these changes Dec 20, 2024

View reviewed changes

jaclync added 2 commits December 20, 2024 09:36

Minor nit updates.

2a94b52

Update PointOfSaleItemsController's internal subject to `CurrentVal…

00fc51f

…ueSubject` so that it does not require another private variable with didSet.

jaclync mentioned this pull request Dec 20, 2024

[Woo POS] [Variable Products] Use pagination headers for loading products #14728

Merged

6 tasks

jaclync approved these changes Dec 20, 2024

View reviewed changes

joshheald merged commit bfe0243 into trunk Dec 20, 2024
14 of 15 checks passed

joshheald deleted the issue/14693-split-container-and-list-states-with-new-pagination branch December 20, 2024 08:01

jaclync mentioned this pull request Dec 23, 2024

[Woo POS] [Variable Products] Split ItemListState into container and list states #14693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS] Split container and list states with new pagination #14727

[Woo POS] Split container and list states with new pagination #14727

joshheald commented Dec 18, 2024 •

edited by jaclync

Loading

dangermattic commented Dec 18, 2024 •

edited

Loading

wpmobilebot commented Dec 18, 2024 •

edited

Loading

jaclync left a comment

jaclync Dec 19, 2024

joshheald Dec 19, 2024

jaclync Dec 20, 2024

joshheald commented Dec 19, 2024

jaclync left a comment

jaclync Dec 20, 2024

jaclync left a comment

joshheald commented Dec 20, 2024

[Woo POS] Split container and list states with new pagination #14727

[Woo POS] Split container and list states with new pagination #14727

Conversation

joshheald commented Dec 18, 2024 • edited by jaclync Loading

Description

Testing information

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

dangermattic commented Dec 18, 2024 • edited Loading

wpmobilebot commented Dec 18, 2024 • edited Loading

jaclync left a comment

Choose a reason for hiding this comment

jaclync Dec 19, 2024

Choose a reason for hiding this comment

joshheald Dec 19, 2024

Choose a reason for hiding this comment

jaclync Dec 20, 2024

Choose a reason for hiding this comment

joshheald commented Dec 19, 2024

jaclync left a comment

Choose a reason for hiding this comment

jaclync Dec 20, 2024

Choose a reason for hiding this comment

jaclync left a comment

Choose a reason for hiding this comment

joshheald commented Dec 20, 2024

joshheald commented Dec 18, 2024 •

edited by jaclync

Loading

dangermattic commented Dec 18, 2024 •

edited

Loading

wpmobilebot commented Dec 18, 2024 •

edited

Loading