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)

rebase

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