search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.13k stars 2.63k forks source link

[$250] Workspace - Connect bank account one moment page sliding animation is distorted #43920

Closed lanitochka17 closed 4 days ago

lanitochka17 commented 3 weeks ago

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

Version Number: 1.4.85-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Log in with a new expensifail account
  2. Create a workspace
  3. Enable Workflows
  4. Navigate to workspace settings - Workflows
  5. Click on "Connect bank account"

Expected Result:

The animation should be smooth, and without any distortions

Actual Result:

Connect bank account one moment page sliding animation is distorted. The sliding animation is choppy and you can see the text at it's intended position before the animation finishes

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/92468aee-3491-49a6-83b6-a81c8ac44f83

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01747cba36f37ce546
  • Upwork Job ID: 1804218709299744642
  • Last Price Increase: 2024-07-05
Issue OwnerCurrent Issue Owner: @getusha
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 3 weeks ago

@mallenexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

bernhardoj commented 3 weeks ago

Proposal

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

The connect bank account page slide animation isn't smooth.

What is the root cause of that problem?

On the bank account page, we have a navigation code that will be called whenever the step is changed. https://github.com/Expensify/App/blob/c11ae2f8244bbef15c153bfaa8bb707a5d1ab645/src/pages/ReimbursementAccount/ReimbursementAccountPage.tsx#L312

When we open the bank account page, the step is initially empty and will be updated to /new which is mapped to BankAccountStep that shows whether to connect manually or with Plaid.

The navigation code is there to update the step params of the bank account page. However, the navigation code is called while the slide animation is still ongoing which makes it not smooth.

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

Instead of using navigate to update the params, we can use setParams.

Navigation.setParams({stepToOpen: getRouteForCurrentStep(currentStep)});
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @shawnborton (Design), see these Stack Overflow questions for more details.

mallenexpensify commented 3 weeks ago

@shawnborton 👀 plz, I think this is the bug

image
shawnborton commented 3 weeks ago

Yup, agree that it looks like a bug.

shawnborton commented 3 weeks ago

Seems like @bernhardoj has a good solution but let's get an engineer/C+ assigned to review. I also wonder if this is happening in other places too.

melvin-bot[bot] commented 3 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~01747cba36f37ce546

melvin-bot[bot] commented 3 weeks ago

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

mallenexpensify commented 3 weeks ago

Thanks @shawnborton @getusha and @bernhardoj can you dig around to see if you can find other instances we can fix at the same time as this?

@getusha can you review @bernhardoj 's proposal above too plz

getusha commented 3 weeks ago

@bernhardoj, the solution seems to work, but I don't think we have a complete RCA.

Check out this branch: https://github.com/software-mansion-labs/expensify-app-fork/tree/exclude-magic-code-from-last-visited-path. I am not able to reproduce the issue. We need to make sure we fix the root cause if possible.

bernhardoj commented 3 weeks ago

It doesn't happen in https://github.com/software-mansion-labs/expensify-app-fork/tree/exclude-magic-code-from-last-visited-path because navigating to the same page does nothing which is from this PR.

melvin-bot[bot] commented 2 weeks ago

@shawnborton, @mallenexpensify, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

getusha commented 2 weeks ago

Looks like a regression from https://github.com/Expensify/App/pull/43651

bernhardoj commented 2 weeks ago

Before https://github.com/Expensify/App/pull/42335, navigation action to the same screen with the same name and params is executed.

image

After that PR, the action is ignored. https://github.com/Expensify/App/pull/43651 fixed it if the params are different. It's the same case as: https://github.com/Expensify/App/blob/d005e6ec239b74aed37d26e5a584e6a85fce3bc7/src/libs/Navigation/linkTo/index.ts#L84-L93

mallenexpensify commented 2 weeks ago

What's the next step here? @madmax330 and @eh2077 , do you agree with @getusha 's comment above that this looks like a regression from https://github.com/Expensify/App/pull/43651 ?

mallenexpensify commented 2 weeks ago

Reminder to self to check on this before issue payment

melvin-bot[bot] commented 2 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 week ago

@shawnborton @mallenexpensify @getusha this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 week ago

@shawnborton, @mallenexpensify, @getusha Eep! 4 days overdue now. Issues have feelings too...

getusha commented 1 week ago

I am not able to repro this after reverting https://github.com/Expensify/App/pull/43651, will dig into this more tomorrow.

getusha commented 1 week ago

Found another place where this issue is happening.

https://github.com/Expensify/App/assets/75031127/c02014fd-6f71-4efc-ae35-fcc3902653e5

mallenexpensify commented 1 week ago

It's also happening if you click Settings in the RHP too.

https://github.com/Expensify/App/assets/22508304/bee66310-611a-46b2-95c6-dd7278a170bc

Are these the repro steps @getusha? If so, Any reason I shouldn't update the OP with the steps?

  1. Click on a chat with a workspace
  2. click header
  3. click Members or Settings
  4. then back arrow (and repeat last two steps).
bernhardoj commented 1 week ago

The other RHP sliding animation issue is currently caused by the focus trap which is being handled in https://github.com/Expensify/App/issues/44421 I believe. It affects the bank account page too.

But when the focus trap issue is fixed, our issue here will still exist.

melvin-bot[bot] commented 1 week ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

getusha commented 1 week ago

Are these the repro steps @getusha? If so, Any reason I shouldn't update the OP with the steps?

I think one instance is enough, they all seem to have a common root cause.

melvin-bot[bot] commented 1 week ago

@shawnborton, @mallenexpensify, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

mallenexpensify commented 1 week ago

@bernhardoj does your proposal need to be updated before @getusha reviews?

bernhardoj commented 1 week ago

The solution for our issue is still the same, but looks like I can't repro this anymore

getusha commented 6 days ago

I think this got fixed here https://github.com/Expensify/App/issues/44421 we can close this @mallenexpensify (you're assigned there too 😅 )

getusha commented 6 days ago

for this we can close https://github.com/Expensify/App/issues/39463 without payment for C+

mallenexpensify commented 6 days ago

you're assigned there too 😅 )

d'oh, sorry I didn't catch that sooner.

for https://github.com/Expensify/App/issues/43920#issuecomment-2195955423 we can close https://github.com/Expensify/App/issues/39463 without payment for C+

Can you provide more context here, I'm unsure how they're related.

getusha commented 5 days ago

Can you provide more context here, I'm unsure how they're related.

@mallenexpensify I got paid twice (via newDot and upwork) in that issue and we planned to close this without a payment to compensate that.

mallenexpensify commented 4 days ago

Let's keep that issue separate for now @getusha .

Closing