feat: update pull to refresh functionality on wallet and home tabs #6255
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6255 +/- ##
========================================
Coverage 88.92% 88.92%
========================================
Files 737 737
Lines 31431 31436 +5
Branches 5837 5835 -2
========================================
+ Hits 27950 27955 +5
+ Misses 3437 3282 -155
- Partials 44 199 +155
... and 67 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -499,13 +505,11 @@ export default function TransactionFeedV2() { | |||
| scrollEventThrottle={16} | |||
| refreshControl={ | |||
| <RefreshControl | |||
| refreshing={isRefreshingBalances} | |||
| onRefresh={onRefresh} | |||
| refreshing={status === 'loading'} | |||
There was a problem hiding this comment.
@kathaypacific we already have isFetching that signalizes that the feed is refreshing. What do you think of renaming the loading status to loadingBalances and modifying the check to status === 'loadingBalances' && isFetching? It might be a bit more verbose but it will make it more clear why we don't just use isFetching as a source of truth for this check (cause we also need to wait for the balance fetch).
There was a problem hiding this comment.
oh but the loading state for this screen doesn't depend on the balances - we only want to show the loader if the user has pulled to refresh their transaction feed...the main reason for using status === 'loading rather than isFetching is that isFetching is true once every POLL_INTERVAL which causes the screen to be shifted down to show the spinner, which feels very unexpected. i tried to rename the isFetching return variable from the hook that is responsible for the poll to separate the loading state between the poll hook and the fetch next page hook, but it doesn't seem to work 😅
There was a problem hiding this comment.
@kathaypacific gotcha! I haven't thought about the constant layout shift, this makes sense now!
| if (('hasAfterCursor' in error && error.hasAfterCursor) || feedFirstPage.length === 0) { | ||
| setShowError(true) | ||
| setStatus('error') | ||
| } |
There was a problem hiding this comment.
@kathaypacific what do you think of moving this to a derived state? It seems like the only scenario when we use this error status is the same as it was in the previous revision with showError variable. It might also reduce the complexity of the new status field.
const showRefetchError = useMemo(() => {
if (error === undefined) return false;
return ( 'hasAfterCursor' in error && error.hasAfterCursor) || feedFirstPage.length === 0
}, [error, feedFirstPage])There was a problem hiding this comment.
the main reason i wanted to have this enum value for status rather than have independent states for error and loading is to prevent both states from being true at the same time (and to avoid needing to put in extra logic to sync these 2 statuses) - the behaviour i want is when there is a fetch error, the user pulls to refresh, the loader should be displayed and the error is hidden (since at this point, the error is outdated + it's more clear that there is a new error if the error is first dismissed during loading). the status enum is this is a pattern we use in a few places, like the positions slice.
Description
As the title:
Test plan
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-21.at.16.17.30.mp4
Related issues
Backwards compatibility
Y
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: