Page MenuHomePhabricator

[native] implement onPressSetAvatar function
ClosedPublic

Authored by ginsu on Apr 7 2023, 11:44 AM.
Tags
None
Referenced Files
F2109673: D7359.id25056.diff
Tue, Jun 25, 5:16 PM
Unknown Object (File)
Mon, Jun 24, 12:30 PM
Unknown Object (File)
Mon, Jun 24, 12:48 AM
Unknown Object (File)
Sat, Jun 22, 2:38 PM
Unknown Object (File)
Fri, Jun 14, 1:22 PM
Unknown Object (File)
Fri, Jun 14, 8:51 AM
Unknown Object (File)
Thu, Jun 13, 6:46 PM
Unknown Object (File)
Thu, Jun 13, 11:59 AM
Subscribers

Details

Summary

Implement the onPressSetAvatar function. This diff handles the user experience for when a user presses the save avatar button.

Here's an outline of the experience from a high level:

  1. We attempt to call the update user avatar endpoint
  2. While we are making this attempt, both the save and reset button will be disabled and a spinner will render on top of the emoji avatar
  3. If we are successful we will show a toast message saying the avatar has been updated
  4. If we fail then an alert will display telling the user something went wrong and to try again later

Depends on D7403

Test Plan

Please watch the demo video to see the changes I made

Success case:

Failed case:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested review of this revision.Apr 7 2023, 12:01 PM
native/profile/emoji-avatar-creation.react.js
50 ↗(On Diff #24830)

Initially tried to disable the buttons by checking if loadingStatus === 'loading'; however, didn't seem to do the trick so made this local state to handle disabling when we are saving the avatar

ashoat requested changes to this revision.Apr 7 2023, 6:08 PM
ashoat added inline comments.
native/profile/emoji-avatar-creation.react.js
50 ↗(On Diff #24830)

This is almost certainly because you're creating a new loadingStatusSelector on every render of this component. We don't do this anywhere else in the codebase... that should've been a strong signal to you that you're doing something wrong, especially in light of the issue you were seeing

When you use a new function like this that you're not familiar with, you need to be aware of what you don't know. You likely haven't read the code, and it appears that you haven't inspected its usage closely in other contexts. That should be a "red flag" to you that something could very likely be wrong with your code. As soon as you see an issue (or even better, BEFORE you see an issue) you should immediately suspect that you're doing something wrong, and start looking closely at existing usages and reading the implementation

By papering over this issue and hacking around it, you've likely introduced a second issue. Ask yourself: if loadingStatus === 'loading' doesn't handle disabling the buttons correctly, then why do you think that very same check will perform for deciding whether to display ActivityIndicator? I strongly suspect it will have the same issue

This revision now requires changes to proceed.Apr 7 2023, 6:08 PM
native/profile/emoji-avatar-creation.react.js
50 ↗(On Diff #24830)

Looking over this with a fresh set of eyes, this was pretty stupid of me, and I should have definitely spent some more time looking at other places where we use loadingStatusSelector. Will have an update for this diff shortly

ginsu edited the summary of this revision. (Show Details)

address ashoat's comments

ashoat requested changes to this revision.Apr 10 2023, 1:23 PM
ashoat added inline comments.
native/profile/emoji-avatar-creation.react.js
54 ↗(On Diff #24918)

Typo

85 ↗(On Diff #24918)

I think it would be good to add an exclamation mark

105–118 ↗(On Diff #24918)

I'm concerned about the following scenario:

  1. User opens EmojiAvatarCreation on device A
  2. User messes around with it on device A, sets some pending state but DOES NOT save
  3. User then opens EmojiAvatarCreation on device B and saves a new avatar
  4. User then hits reset on device A. It should reset to the new saved avatar, but instead it resets to the original avatar, which is misleading

I think that we should just have onPressReset reset to the current state in Redux. Then we wouldn't need lastSavedAvatar, and arguably we wouldn't need props.route.params.emojiAvatarInfo either if we instead pass some ID indicating what kind of avatar it is.

It might be somewhat difficult to make the "fetch from Redux" part generic if it has to handles thread and user avatars, which is why I suspect you were using React Navigation params in the first place. This component is in native/profile, so it's not immediately clear to me if you intend on using it for thread avatars too... but if you do, I suspect it might be better to factor out a more generic component that takes a "selection function" prop (that selects the avatar from Redux), and two other components that compose the generic one.

109 ↗(On Diff #24918)

Typo

This revision now requires changes to proceed.Apr 10 2023, 1:23 PM
native/profile/emoji-avatar-creation.react.js
105–118 ↗(On Diff #24918)

I think that we should just have onPressReset reset to the current state in Redux.

This is a good idea.

This component is in native/profile, so it's not immediately clear to me if you intend on using it for thread avatars too... but if you do, I suspect it might be better to factor out a more generic component that takes a "selection function" prop (that selects the avatar from Redux), and two other components that compose the generic one.

Working on this generic component right now and this is helpful thanks for the tip

This revision is now accepted and ready to land.Apr 12 2023, 5:56 PM