-
Notifications
You must be signed in to change notification settings - Fork 114
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] don't set loading state for pull to refresh #15079
Merged
joshheald
merged 7 commits into
trunk
from
issue/14869-don't-set-loading-state-for-pull-to-refresh
Feb 7, 2025
Merged
[Woo POS] don't set loading state for pull to refresh #15079
joshheald
merged 7 commits into
trunk
from
issue/14869-don't-set-loading-state-for-pull-to-refresh
Feb 7, 2025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we pull to refresh, the built in controller shows a loading indicator. If we set our state to `loading`, we also showed a ghost cell at the bottom of the list. On a pull to refresh, it’s unlikely that the list is going to get longer – more likely, the full first page of results will be shown, so it’ll be the same length or shorter. This commit prevents us from showing the ghost cell on refresh.
Previously we had to put our refreshable modifiers higher in the heirarchy than the views they were refreshing, because we changed the state to `loading` when we pulled to refresh. This change of state cancelled the pull to refresh network task. The state change for refresh was removed in 01cfd95fbd01e45e4a63982390787ab00bd15e84, and now we can group the refreshable control more closely with the list view it relates to.
|
…o issue/14869-don't-set-loading-state-for-pull-to-refresh
…o issue/14869-don't-set-loading-state-for-pull-to-refresh
…o issue/14869-don't-set-loading-state-for-pull-to-refresh
Base automatically changed from
issue/14869-hide-cancellation-errors-from-the-user
to
trunk
February 6, 2025 15:58
jaclync
approved these changes
Feb 7, 2025
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.
Thank you for making this change! This has been bothering me mildly, as this happens easily for variation list with few variations. 🚀
WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Jaclyn Chen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #14869
Merge after: #15077
Description
This PR stops us adding a ghost/skeleton cell to the bottom of the item list when we pull to refresh.
When we pull to refresh, the built in controller shows a loading indicator. Previously, we set our state to
loading
, so we also showed a ghost cell at the bottom of the list.On a pull to refresh, it’s unlikely that the list is going to get longer – more likely, the full first page of results will be shown, so it’ll be the same length or shorter.
Previously we had to put our refreshable modifiers higher in the heirarchy than the views they were refreshing, because we changed the state to
loading
when we pulled to refresh. This change of state cancelled the pull to refresh network task. Now we can group the refreshable control more closely with the list view it relates to.Steps to reproduce
This is easiest to test with a small page size, since you can see whether there's a ghost cell on PTR without scrolling down, then triggering the load next page and (correctly) showing a ghost cell for that. However, if the items don't fill the screen the next page load won't trigger. You can use the dynamic type to get around that.
Testing information
Screenshots
pull.to.refresh.without.ghosts.mp4
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: