Page MenuHomePhabricator

[native] extend avatar component to handle images
ClosedPublic

Authored by ginsu on Mar 27 2023, 5:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 8, 6:56 PM
Unknown Object (File)
Fri, Mar 8, 6:56 PM
Unknown Object (File)
Thu, Mar 7, 5:59 AM
Unknown Object (File)
Thu, Mar 7, 5:59 AM
Unknown Object (File)
Thu, Mar 7, 5:59 AM
Unknown Object (File)
Thu, Mar 7, 5:59 AM
Unknown Object (File)
Thu, Mar 7, 5:59 AM
Unknown Object (File)
Tue, Feb 27, 9:29 PM
Subscribers

Details

Summary

Extended avatar component to either render an emoji avatar or image avatar depending on avatarInfo.type.

Since Multimedia only uses type and uri from mediaInfo, I only included those fields in the AvatarMediaInfo type.

Depends on D7245

Test Plan

To test if the image was rendering correctly, I injected the following as the avatarInfo:

const avatarInfo = { type: 'image', uri: 'https://picsum.photos/200' };

Please see the screenshots to see how the image avatar looks for each size:

Profile:

Screenshot 2023-03-27 at 8.40.30 AM.png (1×1 px, 685 KB)

Large:

Screenshot 2023-03-27 at 8.41.40 AM.png (1×1 px, 952 KB)

Small:

Screenshot 2023-03-27 at 8.43.33 AM.png (1×952 px, 570 KB)

Micro:

Screenshot 2023-03-27 at 8.44.27 AM.png (1×952 px, 613 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 27 2023, 5:58 AM
Harbormaster failed remote builds in B17661: Diff 24179!
ginsu retitled this revision from [native] extend avatar component to handle images to [draft][native] extend avatar component to handle images.Mar 27 2023, 6:06 AM
ginsu edited the summary of this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, atul.
native/components/avatar.react.js
53 ↗(On Diff #24179)

Been thinking/trying to figure this out for a while now, and I would appreciate some guidance with the following here... Hopefully, the comment below makes sense and explains my thinking, but lmk if not

55 ↗(On Diff #24179)

It seems like, based on this line in RemoteImage making onLoad optional should be totally possible

59 ↗(On Diff #24179)
ginsu edited the test plan for this revision. (Show Details)

rebase

Resubmitting annotations so it's easier to see on this diff

native/components/avatar.react.js
53 ↗(On Diff #24186)

Been thinking/trying to figure this out for a while now, and I would appreciate some guidance with the following here... Hopefully, the comment below makes sense and explains my thinking, but lmk if not

55 ↗(On Diff #24186)

It seems like, based on this line in RemoteImage making onLoad optional should be totally possible

59 ↗(On Diff #24186)
ginsu requested review of this revision.Mar 27 2023, 6:50 AM
atul requested changes to this revision.Mar 27 2023, 12:03 PM

Is this diff still in [draft] state? Or is it ready to review?

Can you remove [draft] from title and re-request review if it's ready and I'll take a look?

This revision now requires changes to proceed.Mar 27 2023, 12:03 PM

Is this diff still in [draft] state? Or is it ready to review?

There is just one question I still have in the diff I was hoping to get your or @ashoat's 2 cents on. Sorry, I should have made that more apparent in the description. So not ready for review, but I wasn't sure what the best way to show you guys my code was besides putting up a "draft diff". For the future, is there a better way to show my code and ask questions about it than this?

have Avatar component use Multimedia instead of RemoteImage

ginsu retitled this revision from [draft][native] extend avatar component to handle images to [native] extend avatar component to handle images.Mar 29 2023, 4:02 PM
ginsu edited the summary of this revision. (Show Details)
ginsu added a reviewer: bartek.

This looks good. It is compatible with my changes in D7227 and D7232

atul added inline comments.
native/components/avatar.react.js
81–85 ↗(On Diff #24353)

Could maybe do

return shouldRenderAvatars ? avatar : null;

but up to you.

This revision is now accepted and ready to land.Mar 30 2023, 7:48 AM

address atuls comment and rebase before landing