Page MenuHomePhabricator

[native/lib] introduce useENSResolvedAvatar hook in avatar-utils
ClosedPublic

Authored by ginsu on Mar 30 2023, 1:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 28, 7:15 AM
Unknown Object (File)
Thu, Mar 28, 7:15 AM
Unknown Object (File)
Thu, Mar 28, 7:15 AM
Unknown Object (File)
Thu, Mar 28, 7:15 AM
Unknown Object (File)
Thu, Mar 28, 7:14 AM
Unknown Object (File)
Thu, Mar 28, 7:08 AM
Unknown Object (File)
Fri, Mar 8, 7:02 PM
Unknown Object (File)
Tue, Mar 5, 11:21 PM
Subscribers

Details

Summary

This logic will need to be used in both UserAvatars and ThreadAvatar, so I factored out the logic to easily reuse in both cases.

Depends on D7259

Test Plan

Same test plan as D7246, and all user avatars still render correctly

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, atul.
ginsu requested review of this revision.Mar 30 2023, 2:06 PM
This revision is now accepted and ready to land.Mar 30 2023, 4:47 PM
ashoat requested changes to this revision.Mar 30 2023, 5:34 PM

Actually after reading D7262 it took me way too long to understand what was going on here. I think we can improve readability

lib/shared/avatar-utils.js
127 ↗(On Diff #24444)

This is a confusing specification (name, list of params) for this function.

Its real function is ENS resolution. It's given an avatarInfo, and if it finds avatarInfo.type === 'ens', then it looks for the ENS avatar of the provided userID

Several suggestions:

  1. Instead of userID, pass in userInfo or (even better) ensAddress. UserAvatar already fetches userInfo so this would avoid "fetching it twice", and more importantly it makes it clear what it's being used for (especially if it's ensAddress)
  2. We should swap the parameter order. The more important thing here is avatarInfo. Unless it's { +type: 'ens' }, the other parameter is ignored
  3. We should rename the function to eg. useResolvedAvatar or useENSResolvedAvatar
This revision now requires changes to proceed.Mar 30 2023, 5:34 PM

address ashoat's commentsy

lib/shared/avatar-utils.js
129 ↗(On Diff #24452)

Decided to use userInfo as the second parameter over ethAddress since we would have to copy and paste lines 131-139 in both UserAvatar and ThreadAvatar to get the ethAddress. If we still feel strongly about using ethAddress over userInfo I will make the necessary changes

ginsu retitled this revision from [native/lib] introduce useAvatarForUserID hook in avatar-utils to [native/lib] introduce useENSResolvedAvatar hook in avatar-utils.Mar 30 2023, 11:18 PM
This revision is now accepted and ready to land.Mar 31 2023, 5:30 AM