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.36k stars 2.78k forks source link

[$1000] Web - No transition animation when selecting users in the Request money / Split bill flow #25850

Closed izarutskaya closed 1 year ago

izarutskaya commented 1 year 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!

Action Performed:

  1. Click the FAB
  2. Click either Request money or Split bill
  3. Enter any amount
  4. Click the green next button

Expected Result:

There should be a transition animation when opening the selection

Actual Result:

There is no transition animation

Workaround:

Unknown

Platforms:

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

Version Number: v1.3.57-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:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/115492554/38aaaa8c-6959-4ead-8243-d84400d71150

https://github.com/Expensify/App/assets/115492554/95719417-39ef-4193-86d9-f99704590948

Expensify/Expensify Issue URL:

Issue reported by: @joh42

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692047364460929

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01899f9953a92032b7
  • Upwork Job ID: 1694866275443363840
  • Last Price Increase: 2023-08-25
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

izarutskaya commented 1 year ago

Proposal from @joh42

Proposal

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

There is no transition animation when we go to the user selection step in both the Request money and Split bill flows.

What is the root cause of that problem?

The root cause is that we instantly focus the Name, email, or phone number input, preventing the transition animation from finishing.

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

Let me first state that any solution to this issue will require changes to both the MoneyRequestParticipantsSelector and the MoneyRequestParticipantsSplitSelector to make sure this issue is solved for both the Request money and Split bill flows. The general approach for solving this kind of issue is to focus using the ScreenWrapper's onEntryTransitionEnd, as has been discussed on Slack. Here's how to implement that:

  1. Implement ref forwarding for both the MoneyRequestParticipantsSelector and the MoneyRequestParticipantsSplitSelector
  2. If we want to avoid tight coupling by focusing the OptionsSelector directly from the MoneyRequestParticipantsPage, add an imperativeHandler to both the MoneyRequestParticipantsSelector and the MoneyRequestParticipantsSplitSelector, with a function for focusing the OptionsSelector in those components
  3. Add onEntryTransitionEnd to the MoneyRequestParticipantsPage. When this is fired, we check which of the aforementioned selectors are rendered based on the iouType, and then focus the OptionsSelector of that component It's worth noting that the selectors are conditionally rendered, but since the iouType does not change I do not foresee any problems using onEntryTransitionEnd here: https://github.com/Expensify/App/blob/70c525952d33563d88efbaf799b840ab769716ed/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L97-L111 I do not necessarily think this is the cleanest approach however, as it requires adding ref forwarding to two different components. As such, I'll explain an alternate solution below that is worth considering for this specific bug:

    What alternative solutions did you explore? (Optional)

    Alternative 1:

    Instead of implementing ref forwarding, we can pass the ScreenWrapper's didScreenTransitionEnd to both the MoneyRequestParticipantsSelector and the MoneyRequestParticipantsSplitSelector. Then we have a useEffect in both those components solely listening to the didScreenTransitionEnd prop. When it changes to true, we focus the options selector. This is essentially the exact same solution as using onEntryTransitionEnd, but it's cleaner because we do not have to worry about ref forwarding nor which of the selectors is currently rendered. As you can see here, didScreenTransitionEnd works the same way as onEntryTransitionEnd: https://github.com/Expensify/App/blob/70c525952d33563d88efbaf799b840ab769716ed/src/components/ScreenWrapper/index.js#L48-L55 Therefore I actually think this might be the optimal solution here.

    Alternative 2:

    We could pass shouldDelayFocus={true} to the OptionsSelector. This would be a one-line fix, but according to the aforementioned Slack discussion we should move away from this approach to focusing.

    Alternative 3:

    We could use InteractionManager.runAfterInteractions for focusing, but again, like with shouldDelayFocus, this is something that we should move away from.