-
Notifications
You must be signed in to change notification settings - Fork 136
[POS Orders] Pagination #14614
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
[POS Orders] Pagination #14614
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
| selectedOrderId = state.selectedOrderId, | ||
| onOrderSelected = onOrderSelected, | ||
| onEndOfOrdersListReached = onEndOfOrdersListReached, | ||
| canLoadMore = canLoadMore && state.pullToRefreshState != WooPosPullToRefreshState.Refreshing, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like that this is equal to
canLoadMore = state.paginationState == WooPosPaginationState.None &&
state.pullToRefreshState != WooPosPullToRefreshState.Refreshing && state.pullToRefreshState != WooPosPullToRefreshState.Refreshing
comparing the same twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Done in 61e72f3
| } | ||
|
|
||
| if (loadOrdersJob?.isActive == true) { | ||
| loadMoreAfterLoadCompletes = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, queuing another load of requests overcomplicates things and isn’t worth the added complexity. I don’t believe users expects this behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I removed that logic in a62305a
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt
Outdated
Show resolved
Hide resolved
|
|
||
| private fun loadOrders() { | ||
| viewModelScope.launch { | ||
| loadOrdersJob?.cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is done already in the search PR so can be reused after taking the changes from there. Also, keep in mind that pagination should also work when the search is on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I will manage that when merging the work on for the Search
…nation # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14614 +/- ##
============================================
+ Coverage 38.42% 38.43% +0.01%
- Complexity 9739 9745 +6
============================================
Files 2059 2059
Lines 115359 115409 +50
Branches 15357 15368 +11
============================================
+ Hits 44328 44363 +35
- Misses 66922 66926 +4
- Partials 4109 4120 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| is WooPosOrdersState.Content -> { | ||
| val isLoadingMore = state.paginationState == WooPosPaginationState.Loading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not commenting on this before. I think the filtering of when to request another page and when to ignore the "end of list" event should be done in the VM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done in f84c0a8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not quite what I meant. The VM should not expose anything beyond its state. Here is an example - I didn't test it much, but it looks correct
Refactor_pagination_logic_in_WooPosOrdersScreen_and_WooPosOrdersViewModel.patch
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt
Outdated
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModel.kt
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDataSource.kt
Show resolved
Hide resolved
kidinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your here!
Overall, everything looks good. The only thing I think needs to be fixed surely is that pagination doesn’t currently work for search cases
|
Thanks for the review @kidinov! As mentioned in the comments, this PR will be followed by:
|
kidinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I left one refactoring proposal which I think would be nice to implement, please take a look
Part of WOOMOB-1149
Description
With this PR we add pagination to the POS Orders list, loading more pages when the user scrolls down to the bottom.
This PR will be followed by:
Steps to reproduce
With a site with enough POS Orders (more than the page size 25)
The tests that have been performed
See above.
Images/gif
Screen_Recording_20250917_130720_Woo.Dev.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.