Page MenuHomePhabricator
Differential D13101

[native] let user choose farcaster avatar
ClosedPublic
Actions

Authored by varun on Aug 15 2024, 10:53 PM.
Tags
None
Referenced Files
F3367630: D13101.diff
Mon, Nov 25, 3:19 PM
Unknown Object (File)
Wed, Nov 20, 10:58 AM
Unknown Object (File)
Wed, Nov 20, 10:58 AM
Unknown Object (File)
Sat, Nov 9, 8:05 AM
Unknown Object (File)
Fri, Nov 8, 7:03 PM
Unknown Object (File)
Fri, Nov 8, 7:03 PM
Unknown Object (File)
Fri, Nov 8, 7:03 PM
Unknown Object (File)
Fri, Nov 8, 7:01 PM
View All 66 Files
Subscribers
ashoat
tomek

Details

Reviewers
ashoat
tomek
Commits
rCOMMa67f3a61c4fb: [native] let user choose farcaster avatar
Summary

This diff makes it possible for a new user to select a farcaster avatar during registration if they have connected their farcaster account. it also makes it possible for existing users to set the farcaster avatar from the profile tab if there's a connected farcaster account.

Depends on D13100

Test Plan

tested at the end of the stack by successfully selecting farcaster avatar during registration. avatar is displayed immediately upon selection and appears in profile after registration is complete

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Aug 15 2024, 10:53 PM
varun held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 15 2024, 10:53 PM
varun mentioned this in D13102: [native] get farcaster avatar selection working from profile.Aug 15 2024, 10:54 PM
Harbormaster completed remote builds in B31125: Diff 43422.Aug 15 2024, 11:32 PM
varun added a child revision: D13102: [native] get farcaster avatar selection working from profile.Aug 19 2024, 7:24 AM
ashoat added inline comments.Aug 22 2024, 10:49 AM
native/avatars/edit-user-avatar.react.js
62 ↗(On Diff #43422)

Keep hooks on their own line

92 ↗(On Diff #43422)
varun updated this revision to Diff 44309.Sep 18 2024, 7:14 AM

rebase

varun updated this revision to Diff 44312.Sep 18 2024, 7:18 AM

squash

varun edited the test plan for this revision. (Show Details)Sep 18 2024, 7:21 AM
varun added reviewers: ashoat, tomek.
varun published this revision for review.Sep 18 2024, 7:23 AM
varun retitled this revision from [native] let user choose farcaster avatar during registration to [native] let user choose farcaster avatar.
varun edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B31696: Diff 44309.Sep 18 2024, 7:31 AM
Harbormaster completed remote builds in B31699: Diff 44312.Sep 18 2024, 7:43 AM
ashoat requested changes to this revision.Sep 18 2024, 12:09 PM
ashoat added inline comments.
lib/hooks/fc-cache.js
11 ↗(On Diff #44312)

Why does the type that useFCNames take need farcasterAvatarURL? It seems like you're overloading this type to be used in two places, when you should really have two separate types

36 ↗(On Diff #44312)

Extremely confusing to see this here... it seems to imply that before this diff, getCachedFarcasterUserForFID returned just the username, and now it's returning an object including a username. But no changes to getCachedFarcasterUserForFID appear to have been made here.

I did some research and it appears that you completely broke useFCNames in D13098, made no task or note about it, and just snuck in a change to fix it here?

If this is correct: please IMMEDIATELY create an urgent task, and a separate diff that fixes the issue

157–159 ↗(On Diff #44312)

Why do we have a map for a single avatar for a single FID? Looks like you copy-dpaste this from another hook that needed a collection, and didn't think to simplify it...

lib/shared/avatar-utils.js
330–331 ↗(On Diff #44312)

Why do we need to check both of these?

This revision now requires changes to proceed.Sep 18 2024, 12:09 PM
ashoat added inline comments.Sep 18 2024, 12:13 PM
native/avatars/edit-user-avatar.react.js
61–62 ↗(On Diff #44312)

Same question as above: why do we need to check both of these?

I highly suspect you can figure out a better way to handle this. Instead of answering directly why you chose to do this, please reevaluate your approach

varun added inline comments.Sep 18 2024, 12:21 PM
lib/hooks/fc-cache.js
36 ↗(On Diff #44312)

I think I realized this was broken while testing and accidentally amended the commit for this diff instead of the one for D13098. Was not trying to sneak anything in... wouldn't make sense since these diffs were published at the same time

varun updated this revision to Diff 44487.Sep 24 2024, 8:50 AM

address feedback

varun planned changes to this revision.Sep 24 2024, 8:50 AM

moving to planned changes until i've read over this diff

Harbormaster completed remote builds in B31819: Diff 44487.Sep 24 2024, 9:18 AM
varun updated this revision to Diff 44503.Sep 24 2024, 2:44 PM

rebase and publish for review

varun added a parent revision: D13450: [lib] update getAvatarForUser signature.Sep 24 2024, 2:45 PM
varun removed a parent revision: D13100: [native] add farcaster avatar URL to registration workflow.
varun added inline comments.Sep 24 2024, 2:59 PM
native/avatars/user-avatar.react.js
18–20 ↗(On Diff #44503)
Harbormaster completed remote builds in B31831: Diff 44503.Sep 24 2024, 3:04 PM
varun mentioned this in D13456: [web] display farcaster avatar when it's set.Sep 24 2024, 3:25 PM
varun added inline comments.Sep 24 2024, 4:30 PM
lib/hooks/fc-cache.js
144 ↗(On Diff #44503)

this could've been a separate diff but wasn't sure if splitting it out now would be confusing since this diff has already been reviewed

ashoat accepted this revision.Sep 24 2024, 5:37 PM
ashoat added inline comments.
lib/hooks/fc-cache.js
144 ↗(On Diff #44503)

Is this function returning a URL? Would be good for the naming to clarify that

148 ↗(On Diff #44503)
156 ↗(On Diff #44503)
This revision is now accepted and ready to land.Sep 24 2024, 5:37 PM
Closed by commit rCOMMa67f3a61c4fb: [native] let user choose farcaster avatar (authored by varun). · Explain WhySep 25 2024, 7:24 AM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMMa67f3a61c4fb: [native] let user choose farcaster avatar.
varun mentioned this in rCOMMfaee4b8008e0: [web] display farcaster avatar when it's set.

Revision Contents
Changeset List

PathSize
lib/
hooks/
50 lines
shared/
15 lines
types/
17 lines
native/
account/
registration/
16 lines
6 lines
avatars/
4 lines
35 lines

Diff 44309

View Options

lib/hooks/fc-cache.js

Loading...
View Options

lib/shared/avatar-utils.js

Loading...
View Options

lib/types/avatar-types.js

Loading...
View Options

native/account/registration/avatar-selection.react.js

Loading...
View Options

native/account/registration/registration-types.js

Loading...
View Options

native/avatars/avatar-hooks.js

Loading...
View Options

native/avatars/edit-user-avatar.react.js

Loading...