Skip to content
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

[Woo POS] Adjust pagination height threshold. Fix GhostItemCardView rendering when no more items. #14234

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Oct 29, 2024

Part of #14186

Description

This PR addresses 2 items:

  • Adds a threshold to infinite scrolling, so this is triggered when we've scrolled 70% through the current loaded view height
  • Fixes an issue where the GhostItemCardView would always show at the end of the list when there are no more items to fetch from the remote.
Screen.Recording.2024-10-29.at.15.10.05.mov

Testing:

  • For easiness, update the ProductsRemote's Constants.productsPerPage to a lower threshold, like 7 or 10.
  • Enter POS.
  • Scroll down slowly, observe the right side of the list how as you scroll it re-positions itself as loads more items
  • Scroll down fast, observe that if you reach the end of the currently-loaded item list, the temporary GhostItemCardView will appear while loading data
  • Scroll to the end of the list, the GhostItemCardView may appear once, but no more after that.

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

@iamgabrielma iamgabrielma added type: task An internally driven task. feature: POS labels Oct 29, 2024
@iamgabrielma iamgabrielma added this to the 21.0 milestone Oct 29, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Oct 29, 2024

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 Oct 29, 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 Numberpr14234-b529301
Version20.9
Bundle IDcom.automattic.alpha.woocommerce
Commitb529301
App Center BuildWooCommerce - Prototype Builds #11403
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Collaborator

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

Thanks for the work!

I'm not sure if it's connected to this change but something in this behavior breaks after doing pull to refresh:

  1. Pull to refresh triggers full screen reload, rather than reload of table
  2. Ghost cells are no longer shown
Simulator.Screen.Recording.-.iPad.Air.11-inch.M2.-.2024-10-30.at.08.34.59.mp4

@iamgabrielma
Copy link
Contributor Author

Thanks for the review!

Pull to refresh triggers full screen reload, rather than reload of table

I just merged trunk and this should be resolved now 😅

Ghost cells are no longer shown

That's an interesting case, these will render perfectly fine if we just scroll down, but will stop rendering if we pull-to-refresh, and then scroll down.

This seem to happen because when we pull-to-refresh, we load the initial items, and then fetch the next page, but uniqueNewItems.count at that point are zero, so it sets hasMoreItems = false which stops rendering the GhostItemCardView.

I spent a couple of hours but I seem unable to find a way through that works 100% of the times. When we reach the end of the page, since we do not know if there are more pages, if we pull-to-refresh we either break the GhostItemCardView rendering, or we break the loading state and triggers a full screen reload, or we have to keep the "infinite" loading at the end of the last item🤔 I'm a bit at a lost here, so any advice is appreciated @staskus

@iamgabrielma
Copy link
Contributor Author

I spent a couple of hours but I seem unable to find a way through that works 100% of the times. When we reach the end of the page, since we do not know if there are more pages, if we pull-to-refresh we either break the GhostItemCardView rendering, or we break the loading state and triggers a full screen reload, or we have to keep the "infinite" loading at the end of the last item🤔 I'm a bit at a lost here, so any advice is appreciated @staskus

Adding to this, something I will try tomorrow is to compare if the last item fetched on n-1 is same as last item fetched on n, if that's the case we could trigger something like "isLastPage" which would resolve the infinite "loading" loop at the end of scrolling without having to rely on view state. The only issue that I foresee is that it wouldn't update new products added via web by scrolling down, and the merchant would have to pull-to-refresh to see these, so maybe is a compromise we have to take for now.

We do not need to remove the items since we’re re-setting the list entirely on reload
@iamgabrielma
Copy link
Contributor Author

Alright, this took longer than expected but seems to be fixed with one liner by updating state assuming that there always be more items if we reload:

Simulator Screen Recording - iPad Air 11 - iOS 17 5 M2 - 2024-10-31 at 09 13 10

  • When there are more items, ghost card is rendered
  • When reaching the bottom of the view, ghost card is hidden
  • Pull-to-refresh doesn't break behaviour

On a related note, I think we still need to make some improvements on how we handle view state around here that should make future changes more bulletproof

@staskus
Copy link
Collaborator

staskus commented Oct 31, 2024

Thanks for the fixes!

I spent a couple of hours but I seem unable to find a way through that works 100% of the times.

Yes, ideally there should be a stronger relationship with the API that tells us the total number of pages or products. Same with pull to refresh, to understand if there are any updates - if not don't refresh the existing list. Otherwise we need to play around with the view state.

Alright, this took longer than expected but seems to be fixed with one liner by updating state assuming that there always be more items if we reload

Unfortunately, I'm able to reproduce a similar issue 🤔

When we do pull to refresh, sometimes two identical requests happen at the same time. The first one finishes with a list of new items, and quickly after that a second request finishes. However, since it was the same request there were no unique items, and hasMoreItems is set to false.

It happens when productsPerPage is low (7), I think at the same time we make pull to refresh also the threshold is triggered. But it can be reproduced with a higher threshold if you scroll down immediately after scrolling down.

ScreenRecording_10-31-2024.14-43-41_1.MP4

A few other things I noticed:

  1. We keep trying to fetch new items even if we don't show ghost cells anymore, intentional?
  2. We try to request a new cell not only when we go down but also when we scroll up. It's partially related to point number 1 since it only happens when the full list is loaded.

@iamgabrielma iamgabrielma modified the milestones: 21.0, 21.1 Oct 31, 2024
We save and perform an additional check against the lastScrollPosition before fetching the next page, this avoids unnecessary network calls when we’re scrolling up through already loaded products.
If we pull-to-refresh fast enough, the system might attempt to reload while the state is still loading
@iamgabrielma
Copy link
Contributor Author

Thanks again for you review Povilas!

We try to request a new cell not only when we go down but also when we scroll up. It's partially related to point number 1 since it only happens when the full list is loaded.

Good catch, I added an additional check on 117ead6 so we're sure to not perform additional calls when scrolling back up.

When we do pull to refresh, sometimes two identical requests happen at the same time. The first one finishes with a list of new items, and quickly after that a second request finishes. However, since it was the same request there were no unique items, and hasMoreItems is set to false. It happens when productsPerPage is low (7), I think at the same time we make pull to refresh also the threshold is triggered. But it can be reproduced with a higher threshold if you scroll down immediately after scrolling down.

This one is tricky, something I added is an additional check to disallow PTR if state is still loading here: b529301, which was possible up until now if we PTR fast enough. But yes, I can see this happening when the list is short enough, since will load the initial items, and also attempt to load the next page since what's in screen triggers the threshold. And as you say, if there are no "new items" coming back, then hasMoreItems is set to false and remains like that when we scroll down.

Screen.Recording.2024-11-01.at.11.23.13.mov

I think here we need to make one of the compromises we talked about in the project thread: Since the API cannot help us, and as each network call should come back with 100 products (or a good part of them), having a low number of items on screen and this causing the ghost cell not rendering would be a an edge cases and can iterate further as we go. What do you think?

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