feat(points): Add error banner on failure fetching subsequent points history pages #5362
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5362 +/- ##
=======================================
Coverage 86.06% 86.06%
=======================================
Files 735 735
Lines 29952 29962 +10
Branches 5125 5128 +3
=======================================
+ Hits 25778 25787 +9
- Misses 3947 3948 +1
Partials 227 227
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| ctaLabel={t('points.history.pageError.refresh')} | ||
| onPressCta={onPressRefresh} | ||
| withBorder={true} | ||
| style={styles.errorBanner} |
There was a problem hiding this comment.
you'll also need to add something like { paddingBottom: Math.max(insets.bottom, Spacing.Thick24) } here, otherwise this content collides with the bottom bar.
There was a problem hiding this comment.
i didn't notice this before, but it seems like the section headers are sticky? is that expected? the homefeed doesn't do this and i guess it doesn't matter so much but in my screenshot above you can see some clipped content which was unexpected for me
There was a problem hiding this comment.
Hmm, this actually isn't something I've ever experienced on Android -- I wonder if this is a platform-specific thing?
There was a problem hiding this comment.
I ran into this on a different screen. https://reactnative.dev/docs/sectionlist#stickysectionheadersenabled - defaults to true on iOS
| @@ -92,6 +96,16 @@ function PointsHistoryBottomSheet({ forwardedRef }: Props) { | |||
| forwardedRef.current?.close() | |||
| } | |||
|
|
|||
| const onPressRefresh = () => { | |||
There was a problem hiding this comment.
| const onPressRefresh = () => { | |
| const onPressRefreshNextPage = () => { |
actually i wonder if you want to consider combining this with onPressTryAgain? they're doing the same thing and could probably just take a getNextPage as input. we could also consider having the same analytics event with a property for getNextPage (since it's the same user action, to retry the fetch?)
There was a problem hiding this comment.
yeah, good callout, combined them into a single fn that shares an analytics event.
| errorBanner: { | ||
| marginHorizontal: Spacing.Regular16, | ||
| }, |
There was a problem hiding this comment.
I suggest we use absolute positioning (over the content) toward the bottom for this one.
So it doesn't abruptly push the content up when it appears.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…history pages (valora-inc#5362) ### Description Adds in an inline notification error when there's a failure fetching subsequent points history page.s See designs [here](https://www.figma.com/file/rXBDplfMHHqYmuu6EkgMEo/Gamification-experiments?type=design&node-id=1486-6438&mode=design&t=gVtyIzOM0LmxJaDy-4). ### Test plan Unit and manual tested. See video below. https://github.com/valora-inc/wallet/assets/569401/c5cb9fc4-50a3-424c-84f6-dab6fb10f981 ### Related issues - Fixes #[issue number here] ### Backwards compatibility Yes. ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)
Description
Adds in an inline notification error when there's a failure fetching subsequent points history page.s See designs here.
Test plan
Unit and manual tested. See video below.
error-page-state-2024-04-29_12.08.40.mp4
Related issues
Backwards compatibility
Yes.
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: