Page MenuHomePhabricator
Differential D11613

[lib/native] introduce connect farcaster alert handler
ClosedPublic
Actions

Authored by ginsu on Tue, Apr 9, 10:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 26, 11:33 PM
Unknown Object (File)
Tue, Apr 23, 7:26 AM
Unknown Object (File)
Mon, Apr 22, 1:03 PM
Unknown Object (File)
Mon, Apr 22, 11:14 AM
Unknown Object (File)
Sun, Apr 21, 10:44 PM
Unknown Object (File)
Sun, Apr 21, 4:21 PM
Unknown Object (File)
Sat, Apr 20, 1:24 PM
Unknown Object (File)
Sat, Apr 20, 11:44 AM
View All 17 Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
inka
Commits
rCOMM0de3396b7379: [lib/native] introduce connect farcaster alert handler
Summary

This diff introduces ConnectFarcasterAlertHandler component which handles the mechanism to prompt the user to connect to farcaster.

I needed to introduce a new handler because FarcasterDataHandler lives in lib and I needed to be able to use the useNavigation hook.

Linear task: https://linear.app/comm/issue/ENG-7525/extend-farcaster-data-handler-to-handle-navigating-to-the-connect + https://linear.app/comm/issue/ENG-7623/introduce-shouldskipconnectfarcasteralert

Depends on D11601

Test Plan

Please see the demo video below

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu created this revision.Tue, Apr 9, 10:30 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptTue, Apr 9, 10:30 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Tue, Apr 9, 10:33 PM
Harbormaster failed remote builds in B28116: Diff 38972!
ginsu edited the summary of this revision. (Show Details)Tue, Apr 9, 10:37 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, inka.
ginsu added a parent revision: D11601: [native/web] introduce migration to update notifPermissionAlertInfo to alertStore in redux.
ginsu requested review of this revision.Tue, Apr 9, 10:47 PM

will make sure ci passes before landing

lib/utils/push-alerts.js
18 ↗(On Diff #38972)

I intentionally introduced this gate because I figured it would be annoying as a dev to keep getting this alert everyday. I introduced a new linear task to track removing this gate when we are ready to flip the switch with the farcaster integration

https://linear.app/comm/issue/ENG-7662/remove-gate-connect-farcaster-alert-gate

native/components/farcaster-handler.react.js
45 ↗(On Diff #38972)

this half second sleep is intentional. Without it, the bottom sheet immediately flys in when the app opens up and bombards the user with things to do. I felt that this was a bit too much/not the best user experience.

native/root.react.js
301 ↗(On Diff #38972)

Needed to move the handler to be within the navigation container so that I could use the useNavigation hook

ashoat added inline comments.Wed, Apr 10, 8:34 AM
lib/utils/push-alerts.js
18 ↗(On Diff #38972)

I don't think this makes sense. You're concerned about the dev experience, but you have no way to launch this work without introducing the very dev experience issue you're worried about. See my comment on the Linear task

ginsu added a child revision: D11621: [native] fix copy in FarcasterPrompt.Wed, Apr 10, 9:45 AM
ginsu updated this revision to Diff 39025.Wed, Apr 10, 2:28 PM

update

ginsu added inline comments.Wed, Apr 10, 2:29 PM
lib/utils/push-alerts.js
18 ↗(On Diff #38972)

Responded on Linear, but updated the gate to be based on isDev and usingCommServicesAccessToken

ginsu added inline comments.Wed, Apr 10, 2:31 PM
native/components/farcaster-handler.react.js
51 ↗(On Diff #39025)

this 1 second sleep is intentional. Without it, the bottom sheet immediately flys in when the app opens up and bombards the user with things to do. I felt that this was a bit too much/not the best user experience.

native/root.react.js
301 ↗(On Diff #39025)

Needed to move the handler to be within the navigation container so that I could use the useNavigation hook

Harbormaster completed remote builds in B28151: Diff 39025.Wed, Apr 10, 2:44 PM
ashoat added inline comments.Thu, Apr 11, 12:49 PM
native/components/farcaster-handler.react.js
22 ↗(On Diff #39025)

Typo

67 ↗(On Diff #39025)

What's the thinking behind having this render FarcasterHandler? It feels to me like it might make more sense for the two components to be siblings, and to rename this one something more specific to what it's doing

ginsu added inline comments.Fri, Apr 12, 12:16 PM
native/components/farcaster-handler.react.js
67 ↗(On Diff #39025)

Was thinking that it would be cleaner to only have a singular "farcaster handler" component that handles all the farcaster business logic, but sounds like having the two components + renaming this one to be more specific to what farcaster logic is being handled here is a better way to go about it

ginsu updated this revision to Diff 39088.Fri, Apr 12, 12:27 PM

address comments

native/components/connect-farcaster-alert-handler.react.js
50 ↗(On Diff #39088)

this 1 second sleep is intentional. Without it, the bottom sheet immediately flys in when the app opens up and bombards the user with things to do. I felt that this was a bit too much/not the best user experience.

ginsu retitled this revision from [lib/native] introduce farcaster handler to [lib/native] introduce connect farcaster alert handler.Fri, Apr 12, 12:28 PM
ginsu edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B28198: Diff 39088.Fri, Apr 12, 12:42 PM
atul accepted this revision.Mon, Apr 15, 4:33 PM

Code changes look good, would be good to make sure all other notes left by other reviewers are addressed/responded to before landing.

lib/utils/push-alerts.js
9–18 ↗(On Diff #39088)

So we just turned this from arrow function to a "traditional"(?) function?

native/components/connect-farcaster-alert-handler.react.js
50 ↗(On Diff #39088)

We're using "our" BottomSheet component here, right? So things should be identical on iOS and Android and we don't need to go out of our way to see how things look on Android?

This revision is now accepted and ready to land.Mon, Apr 15, 4:33 PM
ginsu added inline comments.Mon, Apr 15, 7:22 PM
lib/utils/push-alerts.js
9–18 ↗(On Diff #39088)

Yea sorry probs should have annotated that, but yea this is just bc of my personal preference

native/components/connect-farcaster-alert-handler.react.js
50 ↗(On Diff #39088)

Yea that's correct

ginsu added a comment.Mon, Apr 15, 7:24 PM

Code changes look good, would be good to make sure all other notes left by other reviewers are addressed/responded to before landing.

Confirmed all points of feedback have been addressed

ginsu updated this revision to Diff 39135.Mon, Apr 15, 7:30 PM

rebase before landing

This revision was landed with ongoing or failed builds.Mon, Apr 15, 7:32 PM
Closed by commit rCOMM0de3396b7379: [lib/native] introduce connect farcaster alert handler (authored by ginsu). · Explain Why
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM0de3396b7379: [lib/native] introduce connect farcaster alert handler.
Harbormaster completed remote builds in B28231: Diff 39135.Mon, Apr 15, 7:49 PM

Revision Contents
Changeset List

PathSize
lib/
utils/
34 lines
native/
components/
69 lines
2 lines

Diff 39137

View Options

lib/utils/push-alerts.js

Loading...
View Options

native/components/connect-farcaster-alert-handler.react.js

Loading...
View Options

native/root.react.js

Loading...