This work has been complete for 9 months, but looks like it was never launched. Flipped DISABLE_CONNECT_FARCASTER_ALERT to false and removed the isDev check in shouldSkipConnectFarcasterAlert. Doesn't seem that annoying to see the bottom sheet when devs log in on mobile, but happy to add the isDev check back if we think that's better.
Details
- Reviewers
ashoat
confirmed that the bottom sheet appears on login now
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
lib/utils/push-alerts.js | ||
---|---|---|
24–25 | I'd probably prefer to keep this |
Ended up testing the connect farcaster flow and it works. A couple things worth noting:
- The connect farcaster bottom sheet appears at the same time as the NUX tips if the user installs the app and logs in. Not sure if this edge case is worth addressing
- The bottom sheet appears at the same time as the SIWE backup message screen when a SIWE user who doesn't have a backup message yet logs in. This seems unlikely to happen since the backup message generation is now part of the registration flow
Feels like there are probably ways to dismiss the modal without setting the fid to null here. Ideally we can catch all methods of closing the modal, but if that's really hard, one alternative would just be to set the fid to null as soon as we show the modal.
The connect farcaster bottom sheet appears at the same time as the NUX tips if the user installs the app and logs in. Not sure if this edge case is worth addressing
Can you clarify:
- Does this occur for every login?
- Alternately, does this occur for every fresh install login?
- Or does it only occur for a user that has not previously responded to the prompt?
If it's 1 or 2 I feel like it's probably worth addressing...
The bottom sheet appears at the same time as the SIWE backup message screen when a SIWE user who doesn't have a backup message yet logs in. This seems unlikely to happen since the backup message generation is now part of the registration flow
That makes sense to me.
native/components/connect-farcaster-bottom-sheet.react.js | ||
---|---|---|
81 ↗ | (On Diff #47409) | Is this different than goBack() in some way? |
96 ↗ | (On Diff #47409) | Are there any other ways to dismiss the modal? For instance, the hardware back button on Android? How can we make sure we catch those? |
Alternately, does this occur for every fresh install login?
Yes, it occurs the first time a user logs in after fresh install
Does it not occur on every single login because the NUX doesn't display on subsequent logins?
That's right. When the NUX is displayed, we write to AsyncStorage so that it doesn't appear again for that install
native/components/connect-farcaster-bottom-sheet.react.js | ||
---|---|---|
81 ↗ | (On Diff #47409) | @ginsu explains here why we use close() instead of goBack(): https://phab.comm.dev/D11598?id=38949#inline-69634 |
native/components/connect-farcaster-bottom-sheet.react.js | ||
---|---|---|
81 ↗ | (On Diff #47409) | How does that explain why we use close() instead of goBack()? He only explains why we have an effect for it, I don't see any discussion of close() vs. goBack() |
native/components/connect-farcaster-bottom-sheet.react.js | ||
---|---|---|
81 ↗ | (On Diff #47409) | Oh yeah... not sure why we need this ref |
Feels like there are probably ways to dismiss the modal without setting the fid to null here. Ideally we can catch all methods of closing the modal, but if that's really hard, one alternative would just be to set the fid to null as soon as we show the modal.
the onClosed callback gets called by BottomSheet's onChange callback:
const onChange = React.useCallback( (index: number) => { if (index === -1) { onClosed(); } }, [onClosed], );
i think this should handle all methods of closing the modal
native/components/connect-farcaster-bottom-sheet.react.js | ||
---|---|---|
96 ↗ | (On Diff #47409) | hmm thinking about this more... maybe we don't need this goBack at all? |
native/components/connect-farcaster-bottom-sheet.react.js | ||
---|---|---|
96 ↗ | (On Diff #47409) | oh we need the goBack to get rid of the backdrop |
Nice find!! Worth confirming that it works for the hardware back button on Android.
The connect farcaster bottom sheet appears at the same time as the NUX tips if the user installs the app and logs in. Not sure if this edge case is worth addressing
Could probably be addressed in a separate diff. If you remove the DISABLE_CONNECT_FARCASTER_ALERT change from this one, we could land this diff sooner and address the NUX tips appearing at the same time later, and then flip DISABLE_CONNECT_FARCASTER_ALERT after that.
native/components/connect-farcaster-bottom-sheet.react.js | ||
---|---|---|
81 ↗ | (On Diff #47409) | Cool let's just swap to goBack() then for simplicity |
96 ↗ | (On Diff #47409) |
We need to call setLocalFID(null) when the modal is dismissed – but I think you got the concern because you responded separately |