Show Loader when adding Workspace Edit #4801

mananjadhav · 2021-08-24T15:28:52Z

Details

Add loading flags for Avatar upload and form submission
IMP: Added isUploading flag to AvatarWithImagePicker with uploadIndicator
Replaced js loop to Animated.loop

Fixed Issues

$ #4487

Tests

Tested by keeping the form contents the same
Updating the workspace name and image
Removing the workspace image
Submitting the form successfully
Uploading a large image

QA Steps

Go to Workspace and click on the Avatar
Check button state when the values are not updated
Change the content/upload the image and see if values are updated
Test Removing the workspace image as well
Submitting the form successfully
Uploading a large image

Tested On

Screenshots

mananjadhav · 2021-08-24T15:35:29Z

@roryabraham I've raised a draft PR. Need to clarify a couple of things:

Should we throw error messages on any form error or avatar upload? or even a success message?
I've noticed when we upload an Avatar image, there isn't any feedback shown to the user. For now, what I've done is added an isUploading flag to AvatarWithImagePicker which shows the Sync icon instead of Pencil when set true. While it did solve the purpose, it doesn't look great. I would recommend an animation similar to AvatartWithIndicator. Shall I go ahead and add it?
If we agree to do the loader thing in Fix spaces #2, a similar UX is missing for the Profile image update as well. Shall I cover that too in this PR if is a 🟢 for the above approach?

roryabraham · 2021-08-24T19:33:32Z

Should we throw error messages on any form error or avatar upload? or even a success message

If there's a form error or API error, we should display a user-friendly error message. A success message would be good too, maybe using Growl or some other visual indication.

I've noticed when we upload an Avatar image, there isn't any feedback shown to the user. For now, what I've done is added an isUploading flag to AvatarWithImagePicker which shows the Sync icon instead of Pencil when set true. While it did solve the purpose, it doesn't look great. I would recommend an animation similar to AvatartWithIndicator. Shall I go ahead and add it?

I'm not sure. I'll defer to @Expensify/design here

If we agree to do the loader thing in Fix spaces #2, a similar UX is missing for the Profile image update as well. Shall I cover that too in this PR if is a 🟢 for the above approach?

I assume you meant item 2 of your list, not the PR linked in the comment. I would think yes 🟢, but again will defer to @Expensify/design here.

Lastly, I'll warn you that what I see in this PR so far violates the Expensify/App Philosophy, specifically UI Binds to Disk. My interpretation of that philosophy is that actions should never really be returning a promise – instead the UI binds to data in Onyx, and calls actions that manage the data. It should never be explicitly waiting for a response.

mananjadhav · 2021-08-24T19:43:21Z

Thanks for the clarification @roryabraham.

If there's a form error or API error, we should display a user-friendly error message. A success message would be good too, maybe using Growl or some other visual indication.

I'll put in a message.

I'm not sure. I'll defer to @Expensify/design here

I'll wait for the comments from the design team.

I assume you meant item 2 of your list, not the PR linked in the comment.

Yes. I meant point# 2 from my list. It seems GH linked to the PR due to #.

Lastly, I'll warn you that what I see in this PR so far violates the Expensify/App Philosophy, specifically UI Binds to Disk.

Thanks for highlighting. I only went ahead with the approach because the same file had the following line:

this.uploadAvatarPromise = uploadAvatar(image).then(url => new Promise((resolve) => {

So should I store these loading props to Onyx?

roryabraham · 2021-08-24T22:23:41Z

So should I store these loading props to Onyx?

For now, I believe this is the approach we'll want to take.

mananjadhav · 2021-08-26T04:36:05Z

Hi Team, any updates on this on the design front?

…space-edit-loader

roryabraham · 2021-08-27T19:16:48Z

@mananjadhav In lieu of input from the design team, I'm going to go ahead and 🟢 both of these proposed changes:

I would recommend an animation similar to AvatartWithIndicator. Shall I go ahead and add it?

And

If we agree to do the loader thing in Fix spaces #2, a similar UX is missing for the Profile image update as well. Shall I cover that too in this PR if is a 🟢 for the above approach?

mananjadhav · 2021-08-27T19:24:05Z

Noted. Thanks for the response @roryabraham. Will update the PR.

…cker

…values

…how error

mananjadhav · 2021-08-30T18:41:31Z

@roryabraham I've updated the PR with the following changes:

Growl messages in catch block
Moving all flags to Onyx
Uploading Indicator in the AvatarImagePicker

I haven't added Growl on success because immediately after the policy is saved, we redirect to workspace card with updated info. Let me know if you still feel we should add a Growl message for success.

…space-edit-loader

roryabraham · 2021-08-30T22:00:33Z

@mananjadhav No growl on success sounds fine to me 👍

stitesExpensify · 2021-08-31T22:23:09Z

src/pages/workspace/WorkspaceEditorPage.js

        this.setState({previewAvatarURL: image.uri});

        // Store the upload avatar promise so we can wait for it to finish before updating the policy
        this.uploadAvatarPromise = uploadAvatar(image).then(url => new Promise((resolve) => {
+            updateLocalPolicyValues(this.props.policy.id, {isAvatarUploading: false});


We can prevent repeating this line by putting in in an always() right?

Can you elaborate this please?

Can we do a chain like

uploadAvatar(image).then(<stuff>).catch(<stuff>).always(updateLocalPolicyValues(this.props.policy.id, {isAvatarUploading: false});)

Got it. Sure. Will update.

src/pages/workspace/WorkspaceEditorPage.js

…space-edit-loader

mananjadhav · 2021-09-02T19:55:44Z

@stitesExpensify @roryabraham I've updated the PR. Additionally, I've also modified the size of the indicator to get it closed to the Avatar and adjusted the icon size.

Screen.Recording.2021-09-03.at.1.23.26.AM.mov

mananjadhav · 2021-09-04T06:26:01Z

PR updated

roryabraham

Thanks @mananjadhav, looks good. If you wanted to update AvatarWithIndicator to use Animated.loop for consistency and a small performance boost that would be great!

Someday I hope we can switch over to using Reanimated instead of React Native's Animated. It comes with much better animation performance and in my opinion a simpler API. But that's a conversation for another day 😄

mananjadhav · 2021-09-05T17:39:09Z

I’ll update the AvatarWithIndicator too.

Yeah I’ve worked on Reanimated v1. But I’ve heard Reanimated V2 last year reduces a lot of boilerplate code too.

… in AvatarIndicator

mananjadhav · 2021-09-05T19:23:34Z

PR updated with AvatarWithIndicator Animation.loop change

roryabraham

Okay, I apologize for not noticing these things earlier, but I have a few more comments:

Start both animations in parallel

I just noticed that there are two animations that are basically happening in parallel, so we should just use Animated.parallel, something like this:

/**
 * Start Animation for Indicator
 *
 * @memberof AvatarWithImagePicker
 */
start() {
    Animated.loop(
        Animated.parallel([
            Animated.timing(this.rotate, {
                toValue: 1,
                duration: 2000,
                easing: Easing.linear,
                isInteraction: false,
                useNativeDriver: true,
            }),
            Animated.spring(this.scale, {
                toValue: 1.666,
                tension: 1,
                isInteraction: false,
                useNativeDriver: true,
            }),
        ]),
    ).start();
}

/**
 * Stop Animation for Indicator
 *
 * @memberof AvatarWithImagePicker
 */
stop() {
    Animated.spring(this.scale, {
        toValue: 1,
        tension: 1,
        isInteraction: false,
        useNativeDriver: true,
    }).start(() => {
        this.rotate.resetAnimation();
        this.scale.resetAnimation();
        this.rotate.setValue(0);
    });
}

DRY up animation code

To do this, I think we should:

Create a new file at src/styles/animations/SpinningIndicatorAnimation.js

That file should export a single class called SpinningIndicatorAnimation:

  import {Animated, Easing} from 'react-native';

  class SpinningIndicatorAnimation {
      constructor() {
          this.rotate = new Animated.Value(0);
          this.scale = new Animated.Value(1);
          this.start = this.start.bind(this);
          this.stop = this.stop.bind(this);
          this.getSyncingStyles = this.getSyncingStyles.bind(this);
      }
  
      /**
       * Start Animation for Indicator
       *
       * @memberof AvatarWithImagePicker
       */
      start() {
          Animated.loop(
              Animated.parallel([
                  Animated.timing(this.rotate, {
                      toValue: 1,
                      duration: 2000,
                      easing: Easing.linear,
                      isInteraction: false,
                      useNativeDriver: true,
                  }),
                  Animated.spring(this.scale, {
                      toValue: 1.666,
                      tension: 1,
                      isInteraction: false,
                      useNativeDriver: true,
                  }),
              ]),
          ).start();
      }
  
      /**
       * Stop Animation for Indicator
       *
       * @memberof AvatarWithImagePicker
       */
      stop() {
          Animated.spring(this.scale, {
              toValue: 1,
              tension: 1,
              isInteraction: false,
              useNativeDriver: true,
          }).start(() => {
              this.rotate.resetAnimation();
              this.scale.resetAnimation();
              this.rotate.setValue(0);
          });
      }
  
      /**
       * Get Indicator Styles while animating
       *
       * @returns {Object}
       */
      getSyncingStyles() {
          return {
              transform: [{
                  rotate: this.rotate.interpolate({
                      inputRange: [0, 1],
                      outputRange: ['0deg', '-360deg'],
                  }),
              },
              {
                  scale: this.scale,
              }],
          };
      }
  }
  
  export default SpinningIndicatorAnimation;

Then both AvatarWithImagePicker and AvatarWithIndicator can just do:
```
constructor(props) {
    super(props);
    this.animation = new SpinningIndicatorAnimation();
    ...
    ...
}
```
And then replace all uses of the animation methods with just this.animation.start(), this.animation.stop(), or this.animation.getSyncingStyles.

Scaling issue

Lastly, I'm looking at the screenshots provided in the PR description, and it doesn't seem like the indicator is getting bigger when the animation starts. Instead, it actually looks like it's getting smaller and is outside the avatar circle.

mananjadhav · 2021-09-07T04:21:08Z

Okay, I'll check the Animation updates and implement them.

Lastly, I'm looking at the screenshots provided in the PR description, and it doesn't seem like the indicator is getting bigger > when the animation starts. Instead, it actually looks like it's getting smaller and is outside the avatar circle.

I had updated this comment but not the PR description. Let me update it in the PR description.

#4801 (comment)

mananjadhav · 2021-09-07T05:24:14Z

@roryabraham There's a comment in AvatarWithIndicator.startRotation,

We need to manually loop the animations as `useNativeDriver` does not work well with `Animated.loop`.

…ation.parallel

mananjadhav · 2021-09-07T21:40:02Z

@roryabraham I am updating the SpinnerAnimation and having the following issue with AvatarWithIndicator, which I can't reproduce in staging.new.expensify.com

If I add a couple of messages in Offline mode, and then disable throttling, the Avatar scale doesn't change back to 1. (stays 1.66 and hence it looks bigger), neither does the rotation stop. I've pushed my change and attaching the video of the issue.

Screen.Recording.2021-09-08.at.3.01.11.AM.mov

I am guessing it is because after this.animation.stop() the component isn't rendered and getIndicatorStyles don't have the updated values of this.scale and this.rotate. I also feel Animated.parallel use must be causing it. I am a little confused that I am unable to reproduce the same in the staging URL where we have the old Animation in place.

and a quick, FYI, the following class (not using Animated.parallel) works fine without any issues.

import {Animated, Easing} from 'react-native';

class SpinningIndicatorAnimation {
    constructor() {
        this.rotate = new Animated.Value(0);
        this.scale = new Animated.Value(1);
        this.startRotation = this.startRotation.bind(this);
        this.start = this.start.bind(this);
        this.stop = this.stop.bind(this);
        this.getSyncingStyles = this.getSyncingStyles.bind(this);
    }

    /**
     * We need to manually loop the animations as `useNativeDriver` does not work well with Animated.loop.
     *
     * @memberof AvatarWithIndicator
     */
    startRotation() {
        this.rotate.setValue(0);
        Animated.timing(this.rotate, {
            toValue: 1,
            duration: 2000,
            easing: Easing.linear,
            isInteraction: false,
            useNativeDriver: true,
        }).start(({finished}) => {
            if (finished) {
                this.startRotation();
            }
        });
    }


    /**
     * Start Animation for Indicator
     *
     * @memberof AvatarWithImagePicker
     * @memberof
     */
    start() {
        this.startRotation();
        Animated.spring(this.scale, {
            toValue: 1.666,
            tension: 1,
            isInteraction: false,
            useNativeDriver: true,
        }).start();
    }

    /**
     * Stop Animation for Indicator
     *
     * @memberof AvatarWithImagePicker
     */
    stop() {
        Animated.spring(this.scale, {
            toValue: 1,
            tension: 1,
            isInteraction: false,
            useNativeDriver: true,
        }).start(() => {
            this.rotate.resetAnimation();
            this.scale.resetAnimation();
            this.rotate.setValue(0);
        });
    }

    /**
     * Get Indicator Styles while animating
     *
     * @returns {Object}
     */
    getSyncingStyles() {
        return {
            transform: [{
                rotate: this.rotate.interpolate({
                    inputRange: [0, 1],
                    outputRange: ['0deg', '-360deg'],
                }),
            },
            {
                scale: this.scale,
            }],
        };
    }
}

export default SpinningIndicatorAnimation;

…p changes

mananjadhav · 2021-09-08T18:07:47Z

@roryabraham Did you get a chance to look at my comments above?

Meanwhile I'll resolve the conflicts.

roryabraham · 2021-09-08T23:44:01Z

@mananjadhav Regarding this comment:

We need to manually loop the animations as useNativeDriver does not work well with Animated.loop.

is there some observable problem that occurs when you use Animated.loop as I suggested above? I looked back at the PR which added that comment and there wasn't any additional context explaining the problem. AFAICT there's nothing that seems to go wrong when I switch to using Animated.loop locally, and it makes the code clearer and more performant. So I'd prefer to do that unless we can somehow expand on how "useNativeDriver does not work well with Animated.loop". Hopefully that makes sense!

We can skip the use of Animated.parallel though since you've found an issue with my suggested implementation.

mananjadhav · 2021-09-09T00:00:32Z

Well, I didn't find any issue on the emulator but I haven't been able to check on a real Android device. Let me see if I can test and accordingly update the PR.

…space-edit-loader # Conflicts: # src/libs/actions/PersonalDetails.js

mananjadhav · 2021-09-09T02:09:40Z

@roryabraham PR updated. I checked on a real Android device and it worked fine. So I am guessing it shouldn't be a problem.

I've also updated the videos with the latest testing of the indicator closer to the image with increased size.

roryabraham

Okay, thanks for making all those changes, this is looking great!

OSBotify · 2021-09-09T15:49:01Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-09-09T22:12:29Z

🚀 Deployed to staging by @roryabraham in version: 1.0.95-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

botify · 2021-09-10T19:12:01Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-17. 🎊

OSBotify · 2021-09-10T19:30:32Z

🚀 Deployed to production by @roryabraham in version: 1.0.96-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mananjadhav added 2 commits August 24, 2021 20:42

fix(wkspace-edit-loader): Added loader for AvatarWithImagePicker

a1d2b38

fix(wkspace-edit-loader): Added loader for avatar + form submission

e90f8ff

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wk…

be3a8b4

…space-edit-loader

roryabraham assigned mananjadhav Aug 27, 2021

mananjadhav added 6 commits August 30, 2021 21:45

fix(wkspace-edit-loader): Added upload indicator to AvatarWithImagePi…

0705612

…cker

fix(wkspace-edit-loader): Added loading indicator to Profile Image

7097670

fix(wkspace-edit-loader): Changed flags from state to onyx props

0e77957

fix(wskpace-edit-loader): Added function for local updates of policy …

d9afbab

…values

fix(wkspace-edit-loader): Removed return Promise and added catch to s…

55025ab

…how error

fix(wkspace-edit-loader): Added catch to throw error

fafe5ec

mananjadhav marked this pull request as ready for review August 30, 2021 18:41

mananjadhav requested a review from a team as a code owner August 30, 2021 18:41

MelvinBot requested review from stitesExpensify and removed request for a team August 30, 2021 18:41

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wk…

93d4769

…space-edit-loader

stitesExpensify requested changes Aug 31, 2021

View reviewed changes

isagoico mentioned this pull request Sep 1, 2021

Puller Bear is not assigning the correct CME when PR is ready for review #4981

Closed

mananjadhav added 3 commits September 3, 2021 00:29

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wk…

048d469

…space-edit-loader

fix(wkspace-editor-loader): Fixed icon size for loader

7713aca

fix(wkspace-edit-loader): Fixed icon sizes in indicator

2d35e86

mananjadhav requested a review from roryabraham September 4, 2021 06:26

roryabraham previously approved these changes Sep 5, 2021

View reviewed changes

fix(wkspace-edit-loader): Changed recursive func call to Animate.loop…

69ca002

… in AvatarIndicator

mananjadhav dismissed roryabraham’s stale review via 69ca002 September 5, 2021 19:22

roryabraham requested changes Sep 6, 2021

View reviewed changes

mananjadhav added 2 commits September 7, 2021 11:10

fix(wkspace-edit-loader): Moved animation to separate class with Anim…

6fcdf7e

…ation.parallel

fix(wkspace-edit-loader): Replace animation with SpinningAnimation

7d51071

fix(wkspace-edit-loader): Rollback Animated.parallel and Animated.loo…

9a5fad8

…p changes

mananjadhav mentioned this pull request Sep 8, 2021

Save button is not disabled even when User has not edited anything in Edit Workspace #4490

Closed

mananjadhav added 2 commits September 9, 2021 06:30

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wk…

cf0de74

…space-edit-loader # Conflicts: # src/libs/actions/PersonalDetails.js

fix(wkspace-edit-loader): Changed to Animated.loop

65f8dfa

mananjadhav requested a review from roryabraham September 9, 2021 02:08

roryabraham approved these changes Sep 9, 2021

View reviewed changes

roryabraham merged commit bc035f5 into Expensify:main Sep 9, 2021

mananjadhav mentioned this pull request Sep 9, 2021

[HOLD for payment 2021-09-17] Workspace - Add Loading UI after user saves profile photo #4487

Closed

OSBotify mentioned this pull request Sep 9, 2021

Deploy Checklist: New Expensify 2021-09-09 #5177

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show Loader when adding Workspace Edit #4801

Show Loader when adding Workspace Edit #4801

mananjadhav commented Aug 24, 2021 •

edited

Loading

mananjadhav commented Aug 24, 2021 •

edited

Loading

roryabraham commented Aug 24, 2021

mananjadhav commented Aug 24, 2021

roryabraham commented Aug 24, 2021

mananjadhav commented Aug 26, 2021

roryabraham commented Aug 27, 2021

mananjadhav commented Aug 27, 2021

mananjadhav commented Aug 30, 2021

roryabraham commented Aug 30, 2021

stitesExpensify Aug 31, 2021

mananjadhav Sep 1, 2021

stitesExpensify Sep 1, 2021

mananjadhav Sep 1, 2021

mananjadhav Sep 2, 2021

mananjadhav commented Sep 2, 2021

mananjadhav commented Sep 4, 2021

roryabraham left a comment

mananjadhav commented Sep 5, 2021

mananjadhav commented Sep 5, 2021 •

edited

Loading

roryabraham left a comment

mananjadhav commented Sep 7, 2021

mananjadhav commented Sep 7, 2021

mananjadhav commented Sep 7, 2021 •

edited

Loading

mananjadhav commented Sep 8, 2021

roryabraham commented Sep 8, 2021

mananjadhav commented Sep 9, 2021

mananjadhav commented Sep 9, 2021 •

edited

Loading

roryabraham left a comment

OSBotify commented Sep 9, 2021

OSBotify commented Sep 9, 2021

botify commented Sep 10, 2021

OSBotify commented Sep 10, 2021

Show Loader when adding Workspace Edit #4801

Show Loader when adding Workspace Edit #4801

Conversation

mananjadhav commented Aug 24, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mananjadhav commented Aug 24, 2021 • edited Loading

roryabraham commented Aug 24, 2021

mananjadhav commented Aug 24, 2021

roryabraham commented Aug 24, 2021

mananjadhav commented Aug 26, 2021

roryabraham commented Aug 27, 2021

mananjadhav commented Aug 27, 2021

mananjadhav commented Aug 30, 2021

roryabraham commented Aug 30, 2021

stitesExpensify Aug 31, 2021

Choose a reason for hiding this comment

mananjadhav Sep 1, 2021

Choose a reason for hiding this comment

stitesExpensify Sep 1, 2021

Choose a reason for hiding this comment

mananjadhav Sep 1, 2021

Choose a reason for hiding this comment

mananjadhav Sep 2, 2021

Choose a reason for hiding this comment

mananjadhav commented Sep 2, 2021

mananjadhav commented Sep 4, 2021

roryabraham left a comment

Choose a reason for hiding this comment

mananjadhav commented Sep 5, 2021

mananjadhav commented Sep 5, 2021 • edited Loading

roryabraham left a comment

Choose a reason for hiding this comment

Start both animations in parallel

DRY up animation code

Scaling issue

mananjadhav commented Sep 7, 2021

mananjadhav commented Sep 7, 2021

mananjadhav commented Sep 7, 2021 • edited Loading

mananjadhav commented Sep 8, 2021

roryabraham commented Sep 8, 2021

mananjadhav commented Sep 9, 2021

mananjadhav commented Sep 9, 2021 • edited Loading

roryabraham left a comment

Choose a reason for hiding this comment

OSBotify commented Sep 9, 2021

OSBotify commented Sep 9, 2021

botify commented Sep 10, 2021

OSBotify commented Sep 10, 2021

mananjadhav commented Aug 24, 2021 •

edited

Loading

mananjadhav commented Aug 24, 2021 •

edited

Loading

mananjadhav commented Sep 5, 2021 •

edited

Loading

mananjadhav commented Sep 7, 2021 •

edited

Loading

mananjadhav commented Sep 9, 2021 •

edited

Loading