-
Notifications
You must be signed in to change notification settings - Fork 137
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
[WooPos][Non Simple Product types] Retry UI for pagination errors on variations screens #12972
base: trunk
Are you sure you want to change the base?
Conversation
…new order button will take us to items list screen instead of variations screen if the user proceeded to checkout while they were in variations screen.
Generated by 🚫 Danger |
📲 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.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## issue/12846-analytics-for-variations #12972 +/- ##
==========================================================================
- Coverage 39.63% 39.63% -0.01%
+ Complexity 5973 5972 -1
==========================================================================
Files 1268 1268
Lines 73276 73285 +9
Branches 10057 10061 +4
==========================================================================
+ Hits 29041 29043 +2
- Misses 41656 41661 +5
- Partials 2579 2581 +2 ☔ View full report in Codecov by Sentry. |
Where does this design come from? IMO, it doesn't match expectations from this kind of UX in a mobile app. I believe this should have:
Something like that maybe? @malinajirka @samiuelson wdyt? |
Wagner gave the designs and we had conversation about this on Slack here IMHO, I don't see any problem with this design and I don't feel it deviates from the mobile UX especially considering that this is run only on tablet devices.
If we show the error as another card with the same height as rest of the items. I can see few confusions that could arise:
What do you think? |
…where it said we support only simple physical products.
I lean towards the approach Andrei mentioned (mostly because it's a standard behavior across apps on Android), but it's ultimately Wagner's decision, so I'd defer this to him. One thing which feels broken is the animation that happens after you tap on |
With the item being so big, I have doubts that people will even understand what's going on, as your synthetical example is not what people will have. They will have a list of 20 or more products. They will scroll down where they will see an item with a shimmer effect, which after some time is replaced by a huge thing, but that huge thing won't be visible automatically. They will see, in most cases, just empty space at the bottom People will need to figure out that they need to scroll down more to find this "retry" button, and there is no necessity at all to have that thing so huge and so not in line with the rest of the items. On top of that the animation looks really wrong when things are that huge. And again, this is completely not what people got used to see in mobile apps, so clarity is really questionable here
But this is not an error screen at all. It's a contextual error. People can easily keep using the shown products if the next page is not loaded
The button in the item can be as huge as we want/need Looking into Figma design, I feel that there is a misunderstanding on the flow. In 2 out of 3 designs, there are no products shown at all. I'll ask Wagner to reconsider |
@@ -8,4 +8,5 @@ interface ContentViewState { | |||
val items: List<WooPosItem> | |||
val loadingMore: Boolean | |||
val reloadingProductsWithPullToRefresh: Boolean | |||
val errorLoadingMoreItems: Boolean |
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.
The state defined like that can contain invalidate states, e.g. loadingMore = true
and errorLoadingMoreItems = true
.
We probably should do something like
val paginationIndicator: PaginationIndicator
enum PaginationIndicator {
Loading, Error, None
}
By the way, ContentViewState
does not follow the naming convention of the top-level classes. It probably should be WooPosItemsViewState.
And there are a few more, e.g., ToolbarAccessibilityLabels
, for instance. And some classes that are in the items
package now, e.g., WooPosBanner
should either contain an item in them or be placed in another package
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 raising this. I had this on my list to refactor hence I kept the PR as draft. Here's the refactored state: a45bd6c
.padding( | ||
start = 16.dp.toAdaptivePadding(), | ||
end = 16.dp.toAdaptivePadding(), | ||
top = 30.dp.toAdaptivePadding(), |
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 30 can not be in 4dp grid. 28 or 32 should be
https://m2.material.io/design/layout/spacing-methods.html#baseline-grid
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.
Done: 71ee602
@@ -100,18 +93,15 @@ class WooPosVariationsViewModel @Inject constructor( | |||
if (!variationsDataSource.canLoadMore()) { | |||
return | |||
} | |||
_viewState.value = currentState.copy(loadingMore = true) | |||
_viewState.value = currentState.copy(loadingMore = true, errorLoadingMoreItems = false) | |||
loadMoreJob?.cancel() | |||
loadMoreJob = viewModelScope.launch { | |||
val result = variationsDataSource.loadMore(productId) | |||
if (result.isSuccess) { | |||
Result.success(Unit) |
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.
What does this line do?
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.
This is no longer relevant as result of this change: a45bd6c
@@ -100,18 +93,15 @@ class WooPosVariationsViewModel @Inject constructor( | |||
if (!variationsDataSource.canLoadMore()) { | |||
return | |||
} | |||
_viewState.value = currentState.copy(loadingMore = true) | |||
_viewState.value = currentState.copy(loadingMore = true, errorLoadingMoreItems = false) | |||
loadMoreJob?.cancel() | |||
loadMoreJob = viewModelScope.launch { | |||
val result = variationsDataSource.loadMore(productId) | |||
if (result.isSuccess) { |
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.
np:
_viewState.value = if (result.isSuccess) {
currentState.copy(loadingMore = false, errorLoadingMoreItems = false)
} else {
currentState.copy(loadingMore = false, errorLoadingMoreItems = true)
}
loadMoreJob?.cancel() | ||
loadMoreJob = viewModelScope.launch { | ||
val result = variationsDataSource.loadMore(productId) | ||
if (result.isSuccess) { | ||
Result.success(Unit) | ||
if (!variationsDataSource.canLoadMore()) { | ||
_viewState.value = currentState.copy(loadingMore = false) |
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.
Can you use currentState here? _viewState.value
can be modified by the time the request comes back but you copying based on the old state
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.
This is no longer relevant as result of this change: a45bd6c
…ean value to minimise the occurence of invalid states
private var loadMoreJob: Job? = null | ||
|
||
fun init(productId: Long) { | ||
fetchVariations(productId = productId, withPullToRefresh = false, withCart = true) | ||
fun init(productId: Long, withPullToRefresh: Boolean = false, withCart: Boolean = 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.
Overall, have you explored the possibility of recreating this VM every time the variations are open? Now it seems very error prone as the state is kept for different lists of variations
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 had and navigating to and from variation screen was causing problem. Problem in the sense that we need only left pane to change when coming back from variations screen but it was looking as if the whole screen was being popped out of the back stack. I've slightly modified and the View Model is recreated now for each variable product: 1718bb3
Looks much better! You show old data for a second before it gets updated now 12-05--15-16.mp4Sometimes it loads the next page but loads nothing: 12-05--15-20.mp4Why page size is 10 items for variations, while for products 25? I know that you reuse the handler, but I am not sure that it should be different in our POS case We won't load the next page if the response hasn't come back yet. It's tricky, but:
12-05--15-38.mp4 |
@@ -42,4 +41,8 @@ class VariationListHandler @Inject constructor(private val repository: Variation | |||
offset += PAGE_SIZE | |||
}.map { } | |||
} | |||
|
|||
fun resetLoadMoreState() { | |||
canLoadMore = false |
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 you modified the atomic var boolean from a different thread, which may lead to inconsistency. Check ProductListHandler
Having said that you may need determine if there is a next page based on specific products. Now it's shared state between different lists which may or already leads to issues
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.
This is no longer needed. I've removed this method.
@kidinov Thanks for your patience and the reviews. Much appreciated 🙇
I couldn't reproduce this to be honest. This should be fixed now with view model being re-created for each variable product
Can you please let me know the exact steps? I couldn't reproduce this either.
I've set page size to 25 now and it should to some extent fix the issue. I'll come up with a separate PR to see how best to avoid this situation. I hope that's okay with you. Also, can you please do another round of testing. Thanks! |
import kotlinx.coroutines.sync.Mutex | ||
import kotlinx.coroutines.sync.withLock | ||
|
||
class VariationsLRUCache<K, V>(maxSize: Int) { |
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 most of the methods of this class are not used
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.
Removed unnecessary methods: f1f54b0
private val cacheMutex = Mutex() | ||
|
||
private suspend fun getCachedVariations(productId: Long): List<ProductVariation> { | ||
return cacheMutex.withLock { variationCache.get(productId) ?: emptyList() } |
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.
You have mutex here and you have another mutex on mutex inside of the cache
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.
Remove unnecessary mutex locks: dfaa6fe
It seems mostly fine now. I left a couple of code-related comments, though And I still have this very weird behavior with the pagination of the variations. Notice that the loading item disappears earlier sometimes, sometimes it doesn't appear at all . I'd suggest you to use https://github.com/woocommerce/wc-smooth-generator plugin and try different cases yourself with different amounts of variations to fix the issue 12-06--15-52.mp4 |
…w state if load more job is active
…tate if load more job is not active
# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/variations/WooPosVariationsScreen.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsViewModel.kt # WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsViewModelTest.kt
@kidinov I was able to figure out the pagination edge case issue. Here's my understanding of the problem: The issue occurs because when I open the variations screen, cached data is displayed first, and a background remote fetch is triggered to refresh the data. If I scroll to the bottom during this time, a loadMore operation is triggered to fetch the next page of variations, and the pagination loading state is displayed. However, when the initial remote fetch completes, it updates the view state and unintentionally resets the pagination loading state, making it look like nothing is happening. This is misleading because the loadMore operation is still in progress, and the user has to wait without any indication until the additional data is finally loaded. This was reproducible on the products list as well. pagination_products.mp4I've made the changes and it should fix the problem. Please have a look. |
I still consistently can reproduce the issue 12-09--21-41.mp4 |
…the variations screen.
@kidinov Can you please have a look now. Let me know if it's still reproducible and I'll probably create a new ticket for it and unblock this PR. Many thanks! |
@AnirudhBhat I could not able to reproduce the issue anymore We'll need to implement the loading more error in this PR, though, as it is also used in products, right? |
Closes: #12967
Description
This PR adds a new UI for retrying errors during paginating variations.
Along with this, this PR also does a couple of small changes:
In totals screen after payment is success. It navigates the screen back to items list screen so that when we click on "new order" button from the payment success screen, we end up on the items list screen instead of variations screen if we had proceeded to totals screen from variations screen.
Changes the messages which previously only mentioned only simple physical products can be added in POS. After this PR, it should say simple physical and variable products can be added. Below are the places where this got changed:
Design
Implementation
Testing information
Retry pagination UI
Navigate to items list after payment success
Changes to messages
Navigate to POS mode (more menu -> POS)
Ensure you see a banner that says both simple and variable products instead of just simple products
Click on learn more in the banner
Ensure the dialog also mentions both simple and variable products
Navigate to POS mode where there are no compatible products to display (no simple and variable products)
Ensure the empty state also says that only simple and variable products are compatible.
The tests that have been performed
Tested on both light and dark modes
Images/gif
pagination_retry_ui.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.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: