Animations for the IOU flow #4070

rdjuric · 2021-07-15T11:13:30Z

Details

Wraps the components of our IOUModal steps in a Animatable.View. Pass a different animation type depending on whether we're increasing or decreasing our currentStepIndex.

Fixed Issues

$ #3921

Tests

QA Steps

Request Money (LHN or Composer) or Split a bill
The flow should be animated

Tested On

Screenshots

Web

web.mov

Mobile Web

mWeb.mp4

Desktop

desktop.mov

iOS

iOS.mp4

Android

android.mp4

rdjuric · 2021-07-15T15:48:35Z

Ready for review @chiragsalian!

chiragsalian

Code LGTM and works well too. Just left some minor style change comments.

chiragsalian · 2021-07-16T00:46:46Z

src/pages/iou/IOUModal.js

+    getAnimation() {
+        if (this.state.previousStepIndex < this.state.currentStepIndex) {
+            return 'slideInRight';
+        } if (this.state.previousStepIndex > this.state.currentStepIndex) {


Let's move this if to the next line. No reason for it to be on the same line since its not an else-if

chiragsalian · 2021-07-16T01:23:45Z

src/pages/iou/steps/IOUParticipantsPage/index.js

                onStepComplete={props.onStepComplete}
                participants={props.participants}
                onAddParticipants={props.onAddParticipants}
            />
        )
        : (
+


remove unnecessary line break

chiragsalian · 2021-07-16T01:24:12Z

src/pages/iou/steps/IOUParticipantsPage/index.js

                onStepComplete={props.onStepComplete}
                onAddParticipants={props.onAddParticipants}
            />
+


same here, let's remove unnecessary line break.

Julesssss

Hi @rdjuric, the animations are looking good but I have a couple of suggestions for improvements:

It is noticeable that the current page disappears when moving to the next page. Could you try to animate the current page out, at the same time that we animate the new page in? This is how it would work natively, so it would be great to match this. It should look we're sliding over to the next page ideally (like this)
Also, it would be great to make the solution a bit more generic so that we can apply the animations to other Modal-with-steps components. Maybe we could wrap the Steps in a new Component (<ModalStep> or <AnimatedStep>?) which would help simplify further usage. It would also prevent the need for defining the animation props (duration/style) in each component which is unecessary.

IOUAnimationTransition.mov

rdjuric · 2021-07-20T20:33:25Z

Thanks for the feedback, @Julesssss

Could you try to animate the current page out, at the same time that we animate the new page in?

This is a bit hard to do here since we're not really changing screens, but just mounting/unmounting components. I modified our logic a bit to only unmount the previous step after we animate it "going away". This way when we click on the next button we first animate the current step out, and then the new step in.

I didn't really like the end result (there's a blank screen for a few ms while the next step renders after the previous one was animated out), but let me know what you think:

For some reason theIOUParticipantsPage is refusing to slideOutRight too, but I'll think of something if we prefer this way.

web.mov

Maybe we could wrap the Steps in a new Component (<ModalStep> or <AnimatedStep>?) which would help simplify further usage.

This was my first try, but wrapping the whole step in the Animatable.View was messing the styling and just didn't work with some components (like our TextInput that we auto-focus on mount).

Julesssss

Hmm, yeah I agree this isn't ideal with the blank screen in the middle.

Honestly, I'm not sure these changes would be a positive improvement without native-looking animations. I believe it should possible, as the react-navigation library is able to achieve it. Here's an example showing it can be achieved, I think the final step would be to find a way to hide the tabs: https://snack.expo.dev/iRsGTO-9h

Please let me know what you think of this solution. An alternative might be to look at the react-navigation code and copy their implementation.

rdjuric · 2021-07-22T14:50:31Z

Hey @Julesssss

Honestly, I'm not sure these changes would be a positive improvement without native-looking animations.

I understand your point! But I also think that I did exactly what was asked/what I proposed on the issue: animated our steps in one direction (slideInLeft) when going forward, and in another (slideInRight) when going backwards.

It's natural that we get a better idea of what we (don't) want when we see one implementation in action, but I feel like the changes you suggest are more related to our logic/navigation system than with the animations.

Here's an example showing it can be achieved, I think the final step would be to find a way to hide the tabs: https://snack.expo.dev/iRsGTO-9h

Please let me know what you think of this solution. An alternative might be to look at the react-navigation code and copy their implementation.

Thanks for the example! Is there any reason we aren't making our steps Screens instead of conditionally rendered components? We also do this in our SignInPage. It feels like we're trying to mimic a Navigator (header, back/close buttons, changing screens, transition animations) without using one, and I think we're hitting the limits of this implementation here.

I think we can:

Change our IOUModal steps to be screens of a navigator, maybe a TabNavigator with tabBarVisibile={false}.
Hack our current implementation to mimic a Navigator more closely?

For me it feels like 1. is a way better solution, and 2. just gets us deeper into the idea of "mimicking a navigator without using one" without (I suppose) any good reason.

Let me know what you think and if you agree this is a different issue.

Julesssss · 2021-07-22T15:19:20Z

I also think that I did exactly what was asked/what I proposed on the issue

Oh for sure, no matter the decision we end up with here, we'll payout for this work. I should have made the desire for native-like animations clear in the initial requirements, my bad.

Is there any reason we aren't making our steps Screens instead of conditionally rendered components?

Yeah, my original intention was to switch to Screens (once react-navigation implemented), but I believe this changed when we realized that this would let web users freely navigate between steps.

My preference would be to go back to your initial solution and to make it more generic and reusable, but let's see if @Expensify/design have a preference between:

Merge the initial animate in/out animation
Merge the latest animate in/out animation
Stick with no animation for now

chiragsalian · 2021-07-26T16:25:28Z

Bump @Expensify/design, on the question asked by Jules.

shawnborton · 2021-07-26T16:32:51Z

I'm a little behind on where this issue stands, but I thought that the whole rightDocked view (header included) would animate from step to step. Right now it feels like just the inner content (below the header) is animating.

Julesssss · 2021-07-26T17:49:17Z

I'm a little behind on where this issue stands, but I thought that the whole rightDocked view (header included) would animate from step to step. Right now it feels like just the inner content (below the header) is animating.

Oh, interesting. I would have thought the opposite. To me, that gives the impression that the steps are individual tasks, rather than a collection of steps with the goal of 'Sending a IOU Request'. The architecture of the Modal also doesn't allow for that animation, because the Header is a separate component which sits above the content -- this isn't impossible, it just seems a bit odd IMO.

I can't point to a specific rule (Material Design peer transitions is the closest I can find), but I think it goes against the typical step-flow mobile UX because the Header (Toolbar/ActionBar) sits above the page hierarchically.

If we were navigating to a new Modal (to add a bank account, for example), then I would totally expect the Header to animate out too -- but because we treat the steps as part of a single Modal, I don't think it makes sense personally.

rdjuric · 2021-07-26T21:40:43Z

It should be possible to animate the header too.

If we were navigating to a new Modal (to add a bank account, for example), then I would totally expect the Header to animate out too -- but because we treat the steps as part of a single Modal, I don't think it makes sense personally.

I have the same impression. Animating only the content gives me the feeling of navigating through the steps of a page, if the header is also animated, I feel more like I'm navigating through pages.

rdjuric · 2021-07-28T13:03:19Z

Any updates here @shawnborton?

shawnborton · 2021-07-28T14:59:29Z

I see your point, so for that sake, it seems like the first screen would animate out entirely as the header changes from screen 1 to screen 2. However, from screen 2 to screen 3, it seems like the header remains the same so I could see an argument to only animate the content there. Does that make sense?

Julesssss · 2021-07-28T15:21:39Z

it seems like the first screen would animate out entirely as the header changes from screen 1 to screen 2. However, from screen 2 to screen 3, it seems like the header remains the same so I could see an argument to only animate the content there. Does that make sense?

I disagree with this too. The fact that the header text changes shouldn't change the steps relationship. The three steps are a group of siblings and sit at the same level hierarchically. This is what allows us to share the state between them architecturally, because the state lives at the IOUModal level (represented by the Header).

I'm pushing back against this because in my opinion, any other navigation than what was implemented here goes against typical mobile UX (because the transition does not match the app navigational hierarchy). If each of the steps was instead an individual Modal, it would totally make sense to animate the full page (and header) out, but this is problematic because this isn't how the stepped-flow was implemented.

This is the best example I can find of the stepped-flow (minus the tabs):

peer-transition.mp4

Julesssss · 2021-09-15T12:40:23Z

src/components/OptionsList.js

-            <View style={this.props.listContainerStyles}>
+            <View
+                style={[...this.props.listContainerStyles]}
+            >


Sorry, just noticed this. This change seems unnecessary too?

Julesssss · 2021-09-15T12:42:17Z

src/pages/iou/IOUModal.js

+        if (this.state.previousStepIndex > this.state.currentStepIndex) {
+            return 'out';
+        }
+        return null;


I don't think we should return null, the Component default is in and this would override it. Let's make in the default here too.

The last return is only called when previousStepIndex === currentStepIndex, we don't want the default in since that would mean the first step (once we open the modal) would animate.

I think we can put this in a (this.state.previousStepIndex === this.state.currentStepIndex) conditional instead? It's the same logic but should be easier to understand.

Ah good point. Yeah, I think that would clear it up a bit. Maybe add a comment too.

rdjuric · 2021-09-15T21:16:41Z

PR updated @Julesssss. Thanks for all the help.

chiragsalian

Looks good to me and works well too. Leaving it to @Julesssss for final review.

chiragsalian · 2021-09-16T22:11:38Z

The issue is labeled n6-hold so I'm going to update the title to reflect the same so that we don't accidentally merge it.

chiragsalian · 2021-09-17T17:16:35Z

As discussed 1:1, placed the title back to [HOLD] so that merging is blocked as GH doesn't recognize [N6-HOLD] to block merging.

Julesssss · 2021-10-18T13:56:00Z

Hi @rdjuric, sorry for the delay, we're now merging PR's again (there is a bonus payment for this delay btw). Would you mind updating the branch once more to resolve the conflict?

chiragsalian · 2021-11-01T19:08:46Z

@rdjuric, friendly bump. Any update here?

chiragsalian · 2021-11-17T01:55:37Z

Looks like there hasn't been an update here in a while so i just took over to push it to the end.
Resolved conflicts, updated with main, retested and GH action tests pass. All yours @Julesssss to review/merge.

Julesssss

Tests well on Web, iOS, Android & mWeb. The animation speed feels a bit slow to me, but we can change this later if others agree.

OSBotify · 2021-11-17T19:41:46Z

✋ 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-11-22T20:53:31Z

🚀 Deployed to staging by @Julesssss in version: 1.1.15-18 🚀

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

OSBotify · 2021-11-29T18:49:06Z

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

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

rdjuric added 7 commits July 15, 2021 08:10

wraps in Animatable.view and getAnimation

b392aee

animation ok

2401b69

lint fixes

31c74e6

comments and removes unecessary changes

0a2672f

return type to comment

2ec237e

improvements to last animation

726421a

more proptypes

05ed316

rdjuric marked this pull request as ready for review July 15, 2021 15:48

rdjuric requested a review from a team as a code owner July 15, 2021 15:48

MelvinBot requested review from chiragsalian and removed request for a team July 15, 2021 15:48

chiragsalian requested changes Jul 16, 2021

View reviewed changes

Julesssss requested changes Jul 16, 2021

View reviewed changes

rdjuric added 2 commits July 19, 2021 19:33

Merge branch 'main' into IOUFlowAnimations

5a4326a

minor changes

e3e24c2

rdjuric requested review from Julesssss and chiragsalian July 20, 2021 20:33

Julesssss requested changes Jul 22, 2021

View reviewed changes

rdjuric requested a review from Julesssss July 22, 2021 14:50

rdjuric requested a review from Julesssss September 15, 2021 11:04

Julesssss reviewed Sep 15, 2021

View reviewed changes

small changes

b3fc7c4

rdjuric requested a review from Julesssss September 15, 2021 21:16

chiragsalian previously approved these changes Sep 16, 2021

View reviewed changes

chiragsalian changed the title ~~Animations for the IOU flow~~ [HOLD] Animations for the IOU flow Sep 16, 2021

Julesssss previously approved these changes Sep 17, 2021

View reviewed changes

Julesssss changed the title ~~[HOLD] Animations for the IOU flow~~ [N6-HOLD] Animations for the IOU flow Sep 17, 2021

chiragsalian changed the title ~~[N6-HOLD] Animations for the IOU flow~~ [HOLD] Animations for the IOU flow Sep 17, 2021

Julesssss changed the title ~~[HOLD] Animations for the IOU flow~~ Animations for the IOU flow Oct 18, 2021

Merge branch 'main' into pr/4070

7ab444b

chiragsalian dismissed stale reviews from Julesssss and themself via 7ab444b November 17, 2021 00:42

correction

a712c94

chiragsalian self-assigned this Nov 17, 2021

Julesssss approved these changes Nov 17, 2021

View reviewed changes

Julesssss merged commit 96ba12d into Expensify:main Nov 17, 2021

OSBotify mentioned this pull request Nov 22, 2021

Deploy Checklist: New Expensify 2021-11-22 #6399

Closed

parasharrajat mentioned this pull request Nov 23, 2021

[HOLD for payment 2021-12-13] [HOLD for payment 2021-12-06] IOU - "Request $xx" button appears at the top of the page #6415

Closed

eVoloshchak mentioned this pull request May 23, 2023

[HOLD for payment 2023-05-16] [$1000] Android - Workspaces - Inscriptions move when clicking back in the process of connecting Bank account #17715

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Animations for the IOU flow #4070

Animations for the IOU flow #4070

rdjuric commented Jul 15, 2021 •

edited

Loading

rdjuric commented Jul 15, 2021

chiragsalian left a comment

chiragsalian Jul 16, 2021

chiragsalian Jul 16, 2021

chiragsalian Jul 16, 2021

Julesssss left a comment •

edited

Loading

rdjuric commented Jul 20, 2021 •

edited

Loading

Julesssss left a comment

rdjuric commented Jul 22, 2021 •

edited

Loading

Julesssss commented Jul 22, 2021

chiragsalian commented Jul 26, 2021

shawnborton commented Jul 26, 2021

Julesssss commented Jul 26, 2021

rdjuric commented Jul 26, 2021

rdjuric commented Jul 28, 2021

shawnborton commented Jul 28, 2021

Julesssss commented Jul 28, 2021

Julesssss Sep 15, 2021 •

edited

Loading

Julesssss Sep 15, 2021

rdjuric Sep 15, 2021

Julesssss Sep 15, 2021

rdjuric commented Sep 15, 2021

chiragsalian left a comment

chiragsalian commented Sep 16, 2021

chiragsalian commented Sep 17, 2021

Julesssss commented Oct 18, 2021

chiragsalian commented Nov 1, 2021

chiragsalian commented Nov 17, 2021

Julesssss left a comment

OSBotify commented Nov 17, 2021

OSBotify commented Nov 22, 2021

OSBotify commented Nov 29, 2021

Animations for the IOU flow #4070

Animations for the IOU flow #4070

Conversation

rdjuric commented Jul 15, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

rdjuric commented Jul 15, 2021

chiragsalian left a comment

Choose a reason for hiding this comment

chiragsalian Jul 16, 2021

Choose a reason for hiding this comment

chiragsalian Jul 16, 2021

Choose a reason for hiding this comment

chiragsalian Jul 16, 2021

Choose a reason for hiding this comment

Julesssss left a comment • edited Loading

Choose a reason for hiding this comment

rdjuric commented Jul 20, 2021 • edited Loading

Julesssss left a comment

Choose a reason for hiding this comment

rdjuric commented Jul 22, 2021 • edited Loading

Julesssss commented Jul 22, 2021

chiragsalian commented Jul 26, 2021

shawnborton commented Jul 26, 2021

Julesssss commented Jul 26, 2021

rdjuric commented Jul 26, 2021

rdjuric commented Jul 28, 2021

shawnborton commented Jul 28, 2021

Julesssss commented Jul 28, 2021

Julesssss Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Julesssss Sep 15, 2021

Choose a reason for hiding this comment

rdjuric Sep 15, 2021

Choose a reason for hiding this comment

Julesssss Sep 15, 2021

Choose a reason for hiding this comment

rdjuric commented Sep 15, 2021

chiragsalian left a comment

Choose a reason for hiding this comment

chiragsalian commented Sep 16, 2021

chiragsalian commented Sep 17, 2021

Julesssss commented Oct 18, 2021

chiragsalian commented Nov 1, 2021

chiragsalian commented Nov 17, 2021

Julesssss left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 17, 2021

OSBotify commented Nov 22, 2021

OSBotify commented Nov 29, 2021

rdjuric commented Jul 15, 2021 •

edited

Loading

Julesssss left a comment •

edited

Loading

rdjuric commented Jul 20, 2021 •

edited

Loading

rdjuric commented Jul 22, 2021 •

edited

Loading

Julesssss Sep 15, 2021 •

edited

Loading