fix(cash-in-bottom-sheet): only show when home is in focus #3927
Conversation
src/home/WalletHome.tsx
Outdated
| if (!isFocused) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
one side effect of this change is that the bottom sheet will now appear every time the home screen gets back focus and the other conditions satisfy, whereas previously it appears only once on app load and then later only once when the balance goes from non zero to zero. I believe this is tracked by local state and this state is kept in memory as long as the conditions to show / not show never changes. With this change though, I believe we lose this state every time focus changes.
There was a problem hiding this comment.
🤔 We could move the cash-in bottom sheet into redux state and dispatch it similar to to the in-app review. That way we only dispatch it on an app action we want such as Actions.SET_ACCOUNT.
There was a problem hiding this comment.
@satish-ravi I ended up wrapping the component in a useMemo which seems to do the trick in 500d92f. I'll test a bit more and confirm things are WAI.
Codecov Report
@@ Coverage Diff @@
## main #3927 +/- ##
=======================================
Coverage 82.27% 82.28%
=======================================
Files 708 708
Lines 26289 26292 +3
Branches 3584 3582 -2
=======================================
+ Hits 21629 21634 +5
+ Misses 4597 4595 -2
Partials 63 63
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| const WrappedCashInBottomSheet = useMemo(() => { | ||
| if (!isFocused) { | ||
| return null |
There was a problem hiding this comment.
This feels a bit too hacky, since isFocused is not in the list of dependencies for useMemo.
Could we instead keep the old implementation and track (via some state) whether we've already displayed the bottom sheet to avoid triggering it every time the screen is put in focus?
There was a problem hiding this comment.
I think we also want to ensure that the bottom sheet will only appear a single time per app session; that is, I think the semantics should be:
- Show the bottom sheet once per app session, on the first occurrence of the following conditions:
- Bottom sheet is enabled
- User has zero balance
- Home screen is focused
Namely, we want to avoid showing the bottom sheet multiple times in the situation where a user goes from zero balance -> nonzero balance -> zero balance, all within the same app session.
Description
Makes is so the cash in bottom sheet is only displayed when the
wallethome.tsxis in focus and re-enables the fiat connect cash out tests: Related Slack Thread.Test plan
Tested locally on Android after the tests are enabled.
Related issues
Backwards compatibility
Yes