Page MenuHomePhabricator

[native] Update `useSelectFromGalleryAndUpdateUserAvatar` to return `isLoading`
ClosedPublic

Authored by atul on Apr 19 2023, 10:56 AM.
Tags
None
Referenced Files
F3150505: D7527.diff
Mon, Nov 4, 11:23 PM
Unknown Object (File)
Fri, Nov 1, 9:13 AM
Unknown Object (File)
Fri, Nov 1, 9:13 AM
Unknown Object (File)
Fri, Nov 1, 9:12 AM
Unknown Object (File)
Fri, Nov 1, 9:09 AM
Unknown Object (File)
Sun, Oct 27, 8:14 AM
Unknown Object (File)
Oct 1 2024, 12:31 PM
Unknown Object (File)
Oct 1 2024, 12:31 PM
Subscribers

Details

Summary

We modify useSelectFromGalleryAndUpdateUserAvatar to return a tuple of [selectFromGalleryAndUpdateUserAvatar, inProgress].

In the next diff we will consume the inProgress returned by useSelectFromGalleryAndUpdateUserAvatar to render a spinner over the image when processing or uploading or updating of avatar is in progress.

We could show some sort of upload progress as well, but I think just a spinner is fine for now.

For convenience, I'm going to have a multiple diffs where I update the EditUserAvatar flow, and then a single diff where I update the EditThreadAvatar flow to match. There's a lot of repetition, but I have ideas on how to consolidate things once we have a shippable experience.

Test Plan

Consume inProgress in EditUserAvatar and log the value. The value being printed matched up with progress of the flow.

28224c.png (266×1 px, 50 KB)

a73f31.png (358×3 px, 103 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/avatars/avatar-hooks.js
149–174 ↗(On Diff #25378)

I have this written down, removing from code because it's just adding clutter for the time being.

168–176 ↗(On Diff #25378)

Moved call to processSelectedMedia into a try/catch block to make sure that if there are any exceptions we still correctly set processingOrUploadInProgress to false.

atul published this revision for review.Apr 19 2023, 10:59 AM
atul edited the test plan for this revision. (Show Details)

Wouldn't it be more simple / readable to handle this at the callsite for selectFromGalleryAndUpdateUserAvatar? Eg. like this:

const [userAvatarUpdateInProgress, setUserAvatarUpdateInProgress] = React.useState(false);
const updateCallback = React.useCallback(async () => {
  setUserAvatarUpdateInProgress(true);
  try {
    const result = await selectFromGalleryAndUpdateUserAvatar(params);
  } finally {
    setUserAvatarUpdateInProgress(false);
  }
}. [selectFromGalleryAndUpdateUserAvatar, params]);

It will lead to a little bit of copy-paste, but might be worth it for the improved readability / simplicity. What do you think?

Wouldn't it be more simple / readable to handle this at the callsite for selectFromGalleryAndUpdateUserAvatar? Eg. like this:

const [userAvatarUpdateInProgress, setUserAvatarUpdateInProgress] = React.useState(false);
const updateCallback = React.useCallback(async () => {
  setUserAvatarUpdateInProgress(true);
  try {
    const result = await selectFromGalleryAndUpdateUserAvatar(params);
  } finally {
    setUserAvatarUpdateInProgress(false);
  }
}. [selectFromGalleryAndUpdateUserAvatar, params]);

It will lead to a little bit of copy-paste, but might be worth it for the improved readability / simplicity. What do you think?

If it was just selectFromGalleryAndUpdateUserAvatar I agree that this solution would be cleaner.

However, we also will need to handle useRemoveUserAvatar and captureImageAndUpdateUserAvatar... and so having a wrapper in EditUserAvatar and EditThreadAvatar for each of those could get repetitive.

Personally prefer that all of the complexity regarding progress is pushed down to the hooks and that they expose a simple isLoading boolean that can be handled from the EditUserAvatar and EditThreadAvatar components. I'm trying to keep as much complexity and logic as possible in the hooks for now to make consolidation of common logic easier once things are shippable.

Also worth noting that even after we await selectFromGalleryAndUpdateUserAvatar that dispatchActionPromise(..., updateUserAvatarCall(imageAvatarUpdateRequest)) will almost definitely still be "loading", so we'd need to have logic to handle that in the Edit[User/Thread]Avatar components.

Okay, good points – that makes sense

This revision is now accepted and ready to land.Apr 19 2023, 11:26 AM