[HOLD #17662] [$1000] Double sliding animation on Send/Request money #17710

kavimuru · 2023-04-20T13:14:56Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

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

Additionally:

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

bug.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?

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

bandicam.2023-04-19.14-36-34-321.mp4

Recording.284.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 · 2023-04-20T13:15:02Z

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

MelvinBot · 2023-04-20T13:15:06Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
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
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.
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.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

PrashantMangukiya · 2023-04-20T15:02:21Z

@mountiny @luacmartins Just sharing my thoughts for this.

It looks like this is regression from this PR #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 · 2023-04-20T20:59:28Z

Yeah this is a valid problem

MelvinBot · 2023-04-20T20:59:40Z

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

MelvinBot · 2023-04-20T20:59:43Z

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

MelvinBot · 2023-04-20T20:59:51Z

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

MelvinBot · 2023-04-20T20:59:57Z

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

PrashantMangukiya · 2023-04-20T21:12:46Z

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.

App/src/pages/iou/MoneyRequestModal.js

Lines 375 to 380 in 1cd304c

 {!didScreenTransitionEnd && <FullScreenLoadingIndicator />} 

 {didScreenTransitionEnd && ( 

 <> 

 {currentStep === Steps.MoneyRequestAmount && ( 

 <AnimatedStep 

 direction={direction}

During component load here is the state for previousStepIndex and currentStepIndex

App/src/pages/iou/MoneyRequestModal.js

Lines 116 to 117 in 1cd304c

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

 const [currentStepIndex, setCurrentStepIndex] = useState(0);

Directions are decided based on the functions below:

App/src/pages/iou/MoneyRequestModal.js

Lines 166 to 189 in 1cd304c

 const direction = useMemo(() => { 

 // If we're going to the "amount" step from the "confirm" step, push it in and pop it out like we're moving 

 // forward instead of backwards. 

 const amountIndex = _.indexOf(steps, Steps.MoneyRequestAmount); 

 const confirmIndex = _.indexOf(steps, Steps.MoneyRequestConfirm); 

 if (previousStepIndex === confirmIndex && currentStepIndex === amountIndex) { 

 return 'in'; 

 } 

 if (previousStepIndex === amountIndex && currentStepIndex === confirmIndex) { 

 return 'out'; 

 } 

 if (previousStepIndex < currentStepIndex) { 

 return 'in'; 

 } 

 if (previousStepIndex > currentStepIndex) { 

 return 'out'; 

 } 

 // Doesn't animate the step when first opening the modal 

 if (previousStepIndex === currentStepIndex) { 

 return null; 

 } 

 }, [previousStepIndex, currentStepIndex, steps]);

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

Web.mp4

luacmartins · 2023-04-20T22:11:51Z

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

MitchExpensify · 2023-04-20T23:00:32Z

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

chiragxarora · 2023-04-20T23:12:37Z

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 · 2023-04-20T23:13:55Z

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

chiragxarora · 2023-04-20T23:18:12Z

Done!

s77rt · 2023-04-21T01:56:24Z

@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 · 2023-04-21T01:58:39Z

@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 · 2023-04-21T09:06:55Z

Proposal

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

There're 2 problems here regarding the animation:

Double sliding animation on Send/Request Money
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?

Here

App/src/pages/iou/MoneyRequestModal.js

Line 178 in 214658e

if (previousStepIndex < currentStepIndex) {

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.
Here https://github.com/Expensify/App/blob/214658eca48517fc3c2d087f569187ddec173ee7/src/pages/iou/MoneyRequestModal.js#L171-174 and here

App/src/pages/iou/MoneyRequestModal.js

Line 174 in 214658e

if (previousStepIndex === amountIndex && currentStepIndex === confirmIndex) {

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?

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

App/src/pages/iou/MoneyRequestModal.js

Line 166 in 214658e

const direction = useMemo(() => {

.

If true, it will return null so there's no animation, and the outdated condition here

App/src/pages/iou/MoneyRequestModal.js

Line 186 in 214658e

if (previousStepIndex === currentStepIndex) {

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

App/src/pages/iou/MoneyRequestModal.js

Line 234 in 214658e

if (currentStepIndex <= 0 && previousStepIndex < 0) {

, previousStepIndex -1 is relied on in the navigation logic.

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

App/src/pages/iou/MoneyRequestModal.js

Line 378 in 214658e

{currentStep === Steps.MoneyRequestAmount && (

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 · 2023-04-21T09:06:57Z

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 · 2023-04-21T13:18:01Z

@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.

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

bug.mov

s77rt · 2023-04-21T13:21:40Z

@luacmartins Yes, let's add that.

luacmartins · 2023-04-21T13:35:36Z

Cool, added to the OP.

s77rt · 2023-05-01T09:51:27Z

Still on hold.

puneetlath · 2023-05-03T20:34:46Z

Moving to Weekly while it's on hold.

s77rt · 2023-05-12T23:26:11Z

Still on hold.

s77rt · 2023-05-22T13:16:25Z

Still on hold.

s77rt · 2023-05-30T21:17:08Z

Same ^

s77rt · 2023-06-08T22:50:30Z

Same ^

s77rt · 2023-06-19T16:25:58Z

Same ^ but getting close

puneetlath · 2023-06-20T16:21:01Z

Going to un-assign myself for now, per the new process.

s77rt · 2023-06-24T11:38:44Z

#17662 is resolved. This issue is no longer reproducible. Can we close this? (or do we need to pay the bug reporter in this case?)

chiragxarora · 2023-06-24T11:40:07Z

Yes :)

chiragxarora · 2023-06-28T15:58:40Z

@MitchExpensify this is fixed now

MitchExpensify · 2023-06-30T22:53:17Z

Reporting bonus paid! Thanks everyone

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 20, 2023

melvin-bot bot assigned MitchExpensify Apr 20, 2023

mountiny added the External Added to denote the issue can be worked on by a contributor label Apr 20, 2023

melvin-bot bot changed the title ~~Double sliding animation on Send/Request money~~ [$1000] Double sliding animation on Send/Request money Apr 20, 2023

melvin-bot bot assigned s77rt Apr 20, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 20, 2023

melvin-bot bot assigned puneetlath Apr 20, 2023

dukenv0307 mentioned this issue Apr 21, 2023

[Pay 6/30] [$1000] App gets stuck in infinite loop between cash and amount page when sending money #17637

Closed

6 tasks

melvin-bot bot added the Overdue label May 1, 2023

melvin-bot bot added Overdue and removed Overdue labels May 1, 2023

puneetlath added Weekly KSv2 and removed Daily KSv2 labels May 3, 2023

melvin-bot bot removed the Overdue label May 3, 2023

melvin-bot bot added the Overdue label May 12, 2023

melvin-bot bot removed the Overdue label May 12, 2023

melvin-bot bot added the Overdue label May 22, 2023

melvin-bot bot removed the Overdue label May 22, 2023

melvin-bot bot added the Overdue label May 30, 2023

melvin-bot bot removed the Overdue label May 30, 2023

melvin-bot bot added the Overdue label Jun 8, 2023

melvin-bot bot removed the Overdue label Jun 8, 2023

melvin-bot bot added the Overdue label Jun 19, 2023

melvin-bot bot removed the Overdue label Jun 19, 2023

puneetlath removed their assignment Jun 20, 2023

MitchExpensify closed this as completed Jun 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD #17662] [$1000] Double sliding animation on Send/Request money #17710

[HOLD #17662] [$1000] Double sliding animation on Send/Request money #17710

kavimuru commented Apr 20, 2023 •

edited by melvin-bot bot

Loading

MelvinBot commented Apr 20, 2023

MelvinBot commented Apr 20, 2023 •

edited by MitchExpensify

Loading

PrashantMangukiya commented Apr 20, 2023

mountiny commented Apr 20, 2023

MelvinBot commented Apr 20, 2023

MelvinBot commented Apr 20, 2023

MelvinBot commented Apr 20, 2023

MelvinBot commented Apr 20, 2023

PrashantMangukiya commented Apr 20, 2023

luacmartins commented Apr 20, 2023

MitchExpensify commented Apr 20, 2023

chiragxarora commented Apr 20, 2023 •

edited

Loading

MitchExpensify commented Apr 20, 2023

chiragxarora commented Apr 20, 2023

s77rt commented Apr 21, 2023

s77rt commented Apr 21, 2023 •

edited

Loading

dukenv0307 commented Apr 21, 2023

MelvinBot commented Apr 21, 2023

luacmartins commented Apr 21, 2023

s77rt commented Apr 21, 2023

luacmartins commented Apr 21, 2023

s77rt commented May 1, 2023

puneetlath commented May 3, 2023

s77rt commented May 12, 2023

s77rt commented May 22, 2023

s77rt commented May 30, 2023

s77rt commented Jun 8, 2023

s77rt commented Jun 19, 2023

puneetlath commented Jun 20, 2023

s77rt commented Jun 24, 2023

chiragxarora commented Jun 24, 2023

chiragxarora commented Jun 28, 2023

MitchExpensify commented Jun 30, 2023

[HOLD #17662] [$1000] Double sliding animation on Send/Request money #17710

[HOLD #17662] [$1000] Double sliding animation on Send/Request money #17710

Comments

kavimuru commented Apr 20, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected results:

Actual results:

Workaround:

Platforms:

MelvinBot commented Apr 20, 2023

MelvinBot commented Apr 20, 2023 • edited by MitchExpensify Loading

Bug0 Triage Checklist (Main S/O)

PrashantMangukiya commented Apr 20, 2023

mountiny commented Apr 20, 2023

MelvinBot commented Apr 20, 2023

MelvinBot commented Apr 20, 2023

MelvinBot commented Apr 20, 2023

MelvinBot commented Apr 20, 2023

PrashantMangukiya commented Apr 20, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

luacmartins commented Apr 20, 2023

MitchExpensify commented Apr 20, 2023

chiragxarora commented Apr 20, 2023 • edited Loading

MitchExpensify commented Apr 20, 2023

chiragxarora commented Apr 20, 2023

s77rt commented Apr 21, 2023

s77rt commented Apr 21, 2023 • edited Loading

dukenv0307 commented Apr 21, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

MelvinBot commented Apr 21, 2023

luacmartins commented Apr 21, 2023

s77rt commented Apr 21, 2023

luacmartins commented Apr 21, 2023

s77rt commented May 1, 2023

puneetlath commented May 3, 2023

s77rt commented May 12, 2023

s77rt commented May 22, 2023

s77rt commented May 30, 2023

s77rt commented Jun 8, 2023

s77rt commented Jun 19, 2023

puneetlath commented Jun 20, 2023

s77rt commented Jun 24, 2023

chiragxarora commented Jun 24, 2023

chiragxarora commented Jun 28, 2023

MitchExpensify commented Jun 30, 2023

kavimuru commented Apr 20, 2023 •

edited by melvin-bot bot

Loading

MelvinBot commented Apr 20, 2023 •

edited by MitchExpensify

Loading

chiragxarora commented Apr 20, 2023 •

edited

Loading

s77rt commented Apr 21, 2023 •

edited

Loading