Page MenuHomePhabricator

[native] Process and upload image for image avatar in `EditAvatar`
ClosedPublic

Authored by atul on Apr 17 2023, 6:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 8:36 PM
Unknown Object (File)
Sat, Nov 9, 8:36 PM
Unknown Object (File)
Sat, Nov 9, 8:36 PM
Unknown Object (File)
Sat, Nov 9, 8:36 PM
Unknown Object (File)
Fri, Nov 8, 9:22 PM
Unknown Object (File)
Tue, Nov 5, 2:10 AM
Unknown Object (File)
Oct 5 2024, 10:31 PM
Unknown Object (File)
Oct 5 2024, 10:14 PM
Subscribers

Details

Summary

We're able to process and upload image for image avatar and get back uploadID. However, before we're able to include uploadID in an image avatar update request we'll need additional information about thread/user passed into EditAvatar. It looks like the actual Avatars are being displayed as child of EditAvatar... I wonder if it makes sense to move UserAvatar and ThreadAvatar "within` EditAvatar and display them conditionally based on props?


Depends on D7476

Test Plan

Logged result of processMedia and observed that it was as expected.

Checked uploads table and saw uploaded images:

17de9d.png (192×3 px, 96 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Apr 17 2023, 6:44 PM
atul edited the test plan for this revision. (Show Details)
native/components/edit-avatar.react.js
53 ↗(On Diff #25248)

Note that we aren't explicitly passing in uploadBlob as we are in input-state-container. This is currently using lib/utils/upload-blob:uploadBlob(...) instead of the one in input-state-container that uses react-native-background-upload.

This worked as is... though we probably want to use the same uploadBlob as input-state-container after we get this working end-to-end?

  1. You forgot to annotate the parent of this diff. This has come up before... please always try to annotate your diff dependencies. It would probably be easier for you if you keep a 1-to-1 correspondence between branches and diff stacks, but if you want to do things differently, it's fine as long as you are able to remember to annotate your diff dependencies going forward.
  2. Accepting due to the state of this work, but normally I would request changes here. Please read through all of the comments carefully and address them before landing
    • If you disagree with the comment about returning a Promise and awaiting it, please re-request review
native/components/edit-avatar.react.js
41 ↗(On Diff #25248)

It would be good to explain why we're not using dispatchActionPromise here, as we typically don't allow "naked" server calls without it. In this case, we're following the convention introduced in InputStateContainer, where we don't dispatch Redux actions for these server calls

45 ↗(On Diff #25248)

Why not destructure dimensions here too, given you're using it below?

47 ↗(On Diff #25248)

Why aren't we returning the promise from here? Is it intentional?

53 ↗(On Diff #25248)

Can you create a follow-up task before landing this diff and link it here?

We could probably extract the uploadBlob function in InputStateContainer into a hook

We definitely want to replace react-native-background-upload, but probably with expo-file-system's upload functionality rather than just using fetch in JS. Would be easier to do if we have one shared callsite

53–54 ↗(On Diff #25248)

Is it necessary to include loop: undefined here?

72 ↗(On Diff #25248)

It continues to really frustrate me that you're iterating on a function that has no callsites in the codebase. I strongly, strongly feel that this is an anti-pattern and in any other scenario I would request changes

109 ↗(On Diff #25248)

Why aren't you awaiting this? Was it intentional?

We'll probably eventually need to track the "loading state" somehow, and probably the easiest solution will be to have some React state that is unset after the upload finishes. Given that, I think you should be awaiting here.

This revision is now accepted and ready to land.Apr 18 2023, 5:49 AM
native/components/edit-avatar.react.js
41 ↗(On Diff #25248)

I was matching InputStateContainer. Should we be using dispatchActionPromise here?

45 ↗(On Diff #25248)

Will update

47 ↗(On Diff #25248)

Was going to return promise and await from openPhotoGallery in subsequent diff where we handle processing/uploading/avatar update progress + states (loading/error).

I can update this diff.

53–54 ↗(On Diff #25248)

It can be removed, will update

109 ↗(On Diff #25248)

We handle progress of processing/upload/avatar update + error states in later diff.

I'll just update to add an await here for now.

This revision was landed with ongoing or failed builds.Apr 18 2023, 8:44 AM
This revision was automatically updated to reflect the committed changes.