Page MenuHomePhabricator

[native] let user choose farcaster avatar
ClosedPublic

Authored by varun on Aug 15 2024, 10:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 4:38 AM
Unknown Object (File)
Tue, Nov 26, 4:11 AM
Unknown Object (File)
Tue, Nov 26, 3:30 AM
Unknown Object (File)
Mon, Nov 25, 6:50 PM
Unknown Object (File)
Mon, Nov 25, 5:51 PM
Unknown Object (File)
Mon, Nov 25, 5:43 PM
Unknown Object (File)
Mon, Nov 25, 5:38 PM
Unknown Object (File)
Mon, Nov 25, 3:19 PM
Subscribers

Details

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 held this revision as a draft.
native/avatars/edit-user-avatar.react.js
62 ↗(On Diff #43422)

Keep hooks on their own line

92 ↗(On Diff #43422)
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)
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
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

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

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

rebase and publish for review

native/avatars/user-avatar.react.js
18–20
lib/hooks/fc-cache.js
144

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 added inline comments.
lib/hooks/fc-cache.js
144

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

148
156
This revision is now accepted and ready to land.Sep 24 2024, 5:37 PM