[$1000] Double sliding animation on Send/Request money

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to staging web app
2. go to send/request money either from a chat or from the big green plus icon

Additionally:

1. Request money from the global create
2. Notice that the confirmation page slides from right to left (0:06 in the video), which is correct
3. Now, request money from within a chat
4. Notice that the confirmation page slides from left to right (0:11 in the video), which is wrong

https://user-images.githubusercontent.com/22219519/233649354-d19b6b4f-4f38-4ddc-b4f2-bb6e04af91c7.mov

Expected results:

Single sliding animation like in other components like new chat, new group

Actual results:

Double sliding animation

Confirmation page animation should be consistent.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.2.2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/233377437-11bc2c84-bce9-4e71-9459-ad036d171032.mp4

https://user-images.githubusercontent.com/43996225/233377480-a45f94b0-39ac-4b61-8930-5690ce5c9921.mp4

Expensify/Expensify Issue URL: Issue reported by: @chiragxarora Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681895092997009

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e04e92b347a09af4
• Upwork Job ID: 1649155886500388864
• Last Price Increase: 2023-04-27

[MelvinBot]

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [x] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [x] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[PrashantMangukiya]

@mountiny @luacmartins Just sharing my thoughts for this.

It looks like this is regression from this PR https://github.com/Expensify/App/pull/16919 Which modified src/pages/iou/MoneyRequestModal.js line 116 from this const [previousStepIndex, setPreviousStepIndex] = useState(0); to this const [previousStepIndex, setPreviousStepIndex] = useState(-1);

So this the root cause of this problem.

If we set previousStepIndex default value to 0 it will solve the problem. I tested it. Thank you.

[mountiny]

Yeah this is a valid problem

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01e04e92b347a09af4

[MelvinBot]

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

[MelvinBot]

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[PrashantMangukiya]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Whenever go to send/request money flow, it will show sliding animation two times i.e. first time when initial loader shows, and thereafter when transition end.

What is the root cause of that problem?

We can see at line 375 it shows <FullScreenLoadingIndicator /> until didScreenTransitionEnd thereafter once transition end it will render content based on the 376 and <AnimatedStep> component load based on the steps and using direction. https://github.com/Expensify/App/blob/1cd304c4d9871caa13362d76f1a8a11a940306b7/src/pages/iou/MoneyRequestModal.js#L375-L380

During component load here is the state for previousStepIndex and currentStepIndex https://github.com/Expensify/App/blob/1cd304c4d9871caa13362d76f1a8a11a940306b7/src/pages/iou/MoneyRequestModal.js#L116-L117

Directions are decided based on the functions below: https://github.com/Expensify/App/blob/1cd304c4d9871caa13362d76f1a8a11a940306b7/src/pages/iou/MoneyRequestModal.js#L166-L189

So here during component load it will trigger the third condition i.e. line 178 as shown in code above. So this is the root cause of the problem.

What changes do you think we should make in order to solve the problem?

We can to set previousStepIndex to 0 as show below, it will solve the issue.

// const [previousStepIndex, setPreviousStepIndex] = useState(-1); //  Old
const [previousStepIndex, setPreviousStepIndex] = useState(0);  // Updated

This will solve the issue as shown in result video.

What alternative solutions did you explore? (Optional)

We can keep previousStepIndex and currentStepIndex value as it and add if condition at position shown below, and remove if from last as shown in code below:

const [previousStepIndex, setPreviousStepIndex] = useState(-1);
const [currentStepIndex, setCurrentStepIndex] = useState(0);

const direction = useMemo(() => {
    ...
    if (previousStepIndex === amountIndex && currentStepIndex === confirmIndex) {
        return 'out';
    }
    if (previousStepIndex === -1 && currentStepIndex === 0) { // *** Add if condition here ***
        return null;
    }
    ...
    // Doesn't animate the step when first opening the modal
    // if (previousStepIndex === currentStepIndex) { // *** Remove this if condition as it is not need ***
    //     return null;
    // }
}, [previousStepIndex, currentStepIndex, steps]);

Results

https://user-images.githubusercontent.com/7823358/233392218-8e8cd789-e43a-4190-a961-6733897ec654.mp4

[luacmartins]

The confirmation page animation is also wrong it slides from the right. That should only be the behavior when editing a field.

[MitchExpensify]

@chiragxarora mind applying to the Upwork job for eventual reporting payment? Thank you!

[chiragxarora]

Hi @MitchExpensify Is the job link same for both reporter and solver? Do I have to write anything specific in the proposal? This is my first one here, have no idea

[MitchExpensify]

Nope just apply so I know your Upwork profile, easy! @chiragxarora 👍

[chiragxarora]

Done!

[s77rt]

@PrashantMangukiya Thanks for the proposal. Your RCA makes sense. But I think we need to understand why such change was required.

@yuwenmemon Was there any reason to change the default previousStepIndex value from 0 to -1. Or was that done just to be "correct" and if so any reason why we didn't use null instead of -1.

[s77rt]

@luacmartins ~I'm not sure I understood what you are referring to exactly, can you please attach a video and edit the OP if you think we should handle that too.~ Edit: I got it, but need to initiate the process from the chat. Please update issue body..

[dukenv0307]

Proposal

Please re-state the problem that we are trying to solve in this issue.

There're 2 problems here regarding the animation:

1. Double sliding animation on Send/Request Money
2. The confirmation page animation is wrong when navigation from the MoneyRequestAmount to the MoneyRequestConfirm page (mentioned here)

What is the root cause of that problem?

1. Here https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L178 where we set animation direction to in when previousStepIndex < currentStepIndex, the condition is also true when the previousStepIndex is -1 and currentStepIndex is 0 which is the initial state of the page, causing the double animation.
2. Here https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L171-174 and here https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L174 we're using custom condition to handle the case of navigation from the MoneyRequestConfirm page to the MoneyRequestAmount page when editing the amount. So basically since the step of MoneyRequestAmount is 0, step of MoneyRequestConfirm is 1, if we don't have this condition, when we navigate from the confirm page to the amount page to try to edit the amount, it will show backwards animation. However that condition breaks the animation of forward navigation from amount page to confirm page initially (not editing).

What changes do you think we should make in order to solve the problem?

1. The direction is only relevant if we go from 1 page to another page, meaning we have previousStepIndex >= 0 ( previousStepIndex = -1 means that there's no previous page) So we need to check if previousStepIndex < 0 in the beginning of https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L166.

If true, it will return null so there's no animation, and the outdated condition here https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L186 can be removed since it's not possible for previousStepIndex to be equal to currentStepIndex.

We cannot set previousStepIndex to 0 initially because that will break the amount editing flow. As we can see here https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L234, previousStepIndex -1 is relied on in the navigation logic.

2. To fix this we need to rethink how we're handling the edit amount flow

Currently what we do is:

• Both the set amount and edit amount are step 0
• If we navigate from confirm page to amount page to edit the amount, by right we're moving forward in the navigation but in code we're actually navigating back to step 0. And then we add a number of non-straight-forward conditions like here and here to show the correct animation & navigation. It can be interpreted as "navigating from step 1 to step 0 but please use forward rather than backward animation", ... which is error prone.

I suspect the reason for that is that we're using the same UI for both steps.

We need to change to:

• edit amount and set amount are 2 separate steps (edit amount will be a new step after confirm page )
• In https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L378 we'll show the same UI for both the edit amount and set amount steps
• Then we can remove all the workaround-ish manipulation for the edit amount case, for example here and here, since when navigating from the confirm page to edit amount page, it's regular forward navigation so it will work with regular animation direction and navigation logic
• In here if it's edit amount step we'll go back, if it's set amount step we'll go to next page.

What alternative solutions did you explore? (Optional)

NA

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[luacmartins]

@s77rt here are the steps to reproduce and a video. If we agree that it's an issue, I can edit the OP to include this.

1. Request money from the global create
2. Notice that the confirmation page slides from right to left (0:06 in the video), which is correct
3. Now, request money from within a chat
4. Notice that the confirmation page slides from left to right (0:11 in the video), which is wrong

https://user-images.githubusercontent.com/22219519/233645746-0fdad575-315d-4b65-a13a-d14cf9a87fe8.mov

[s77rt]

@luacmartins Yes, let's add that.

[luacmartins]

Cool, added to the OP.

[s77rt]

@PrashantMangukiya Please update your proposal to cover the wrong animation bug as well.

[PrashantMangukiya]

@s77rt I am busy with other work so not enough time to research and update proposal. You can go with other proposal. Thank you.

[s77rt]

@dukenv0307 Thanks for the proposal.

Double animation bug

The RCA is correct. But the suggested fix is already proposed by @PrashantMangukiya as an alternative solution.

You mentioned that setting previousStepIndex back to 0 will cause a bug here https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L234. I think the extended if condition is redundant and we should just check the check currentStepIndex since the backward navigation is also guarded by onBackButtonPress here https://github.com/Expensify/App/blob/a33b7193195a84e11e34491940d49e73c1a50ca4/src/pages/iou/MoneyRequestModal.js#L366

Wrong animation bug

The RCA is correct. However the suggested fix looks too much. Can't we differentiate between the two cases:

1. Loading Confirmation page after we set the amount initially
2. Loading Confirmation page after we edit the amount

[dukenv0307]

The RCA is correct. However the suggested fix looks too much. Can't we differentiate between the two cases

@s77rt We can do the following:

1. Apply step 1, 2 in the changes mentioned here https://github.com/Expensify/App/issues/17637#issuecomment-1515824507
2. If we go back from the MoneyRequestConfirm to the MoneyRequestAmount set initially page, we set isEditingAmountAfterConfirm state to false.
3. That isEditingAmountAfterConfirm state can be used to distinguish the case of setting initially vs editing
4. In here, we need to make sure to only override the navigateToPreviousStep if both isEditingAmountAfterConfirm is true and currentStepIndex is 0.

By doing this we might be adding even more ad-hoc logic here to patch the behavior. While I think the initial proposal will fix it at the root by implementing it correctly (it will also fix this issue).

[s77rt]

@dukenv0307 Thanks for the update. I'm not sure if the solution is complete, will this work correctly if we edit the amount and press 'Save' instead of going back? Your proposal seems to only handle the case of navigating to the Edit step which is helpful to fix the linked issue, but for this one I think we also need to handle the case of navigating to the Confirmation step. Can you please update your proposal answering the following questions:

1. Navigating to MoneyRequestAmount. How can we know if we are:
   1. Setting the amount (init open / back press)
   2. Editing the amount (coming from MoneyRequestConfirm)
2. Navigating to MoneyRequestConfirm. How can we know if we are:
   1. Coming directing after setting the amount
   2. Coming after editing the amount

---

The bug we are trying to fix here is a little tricky and while I think your initial proposal may be easier to handle, it does not look logically correct. Editing the amount means going back to the first step and not going forward into a new step. Do you think we can make some further changes to make the proposal more logical?

---

Please do not link parts of the proposal to another. It would be much clearer if you post all the suggested changes here since the other proposal may change and cause confusion.

[dukenv0307]

@s77rt thanks for the review!

answering the following questions:

1. Navigating to MoneyRequestAmount. We know if we are: i. Setting the amount (init open / back press) - If the isEditingAmountAfterConfirm is false ii. Editing the amount (coming from MoneyRequestConfirm) - If the isEditingAmountAfterConfirm is true
2. Navigating to MoneyRequestConfirm. We know if we are: i. Coming directing after setting the amount - If the isEditingAmountAfterConfirm is false ii. Coming after editing the amount - If the isEditingAmountAfterConfirm is true

Let me share an example of the user interaction

• User triggers requesting money and land on MoneyRequestAmount
• User clicks Next to confirm the amount, and now comes to the MoneyRequestConfirm. At this point isEditingAmountAfterConfirm is false, so we know it's case 2.i. to show the correct animation
• User clicks Amount to edit the amount, we know that so we set isEditingAmountAfterConfirm to true -> case 1.ii.
• User clicks either 'Save', or click go back, both lands on the MoneyRequestConfirm. We don't do anything to isEditingAmountAfterConfirm, it's still true -> always case 2.ii.
• User clicks go back, we know they're coming back to the initial MoneyRequestAmount, so we set isEditingAmountAfterConfirm to false -> case 1.i.

These are all the possible interactions between the Amount and Confirm page

[s77rt]

@dukenv0307 Thanks for the explanation. It seems that handling that state is a little messy. I would prefer if we can fix this in another way.

[dukenv0307]

@s77rt sure, that's what I suggested in the initial proposal

The bug we are trying to fix here is a little tricky and while I think your initial proposal may be easier to handle, it does not look logically correct. Editing the amount means going back to the first step and not going forward into a new step. Do you think we can make some further changes to make the proposal more logical?

"Editing the amount means going back to the first step and not going forward into a new step" I think going forward into a new step is correct. Since:

• The animation is forward navigation
• The title text, confirm button text are different in editing amount flow compared to setting amount flow
• We cannot further navigate when we're in editing amount flow, we can only go back
• We would agree that editing "Description" in the confirm page is going forward, editing "Amount" should be the same since it's the same edit-style UI item in the screen.

If it means going back, we should have made it backward navigation and same copies for the confirm button and title.

I think the issues that we have are due to we handling it as backward to previous step and add ad-hoc logic to patch the behavior, while it should be forward to a new step.

[s77rt]

@dukenv0307 Your reasoning here have some valid points :+1:. But I'm not really sure how we are going to move forward given that we have about 3, 4 other issues revolving around the same page. We may need to bring this to Slack.