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.48k stars 2.84k forks source link

[HOLD for payment 2023-08-02] [$1000] Request Money input is not getting focus when navigating back #22610

Closed kavimuru closed 1 year ago

kavimuru 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. Request Money
  2. Enter Amount & Click Next
  3. See focus getting applied to input
  4. Click on any User contact
  5. Come back and see Input is not getting focus.

    Expected Result:

    Input should get focused in any case of navigation.

    Actual Result:

    Not getting in case of navigating back.

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.3.39-5 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/43996225/5246bfd9-9348-4cee-b5f1-4728eb4b900f

https://github.com/Expensify/App/assets/43996225/234b2b53-c0b9-4383-a169-ae8ca4c2caf7

Expensify/Expensify Issue URL: Issue reported by: @Mahesh-Vagicherla

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01612a76428b498945
  • Upwork Job ID: 1679156782678679552
  • Last Price Increase: 2023-07-12
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @maddylewis (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)

tienifr commented 1 year ago

Proposal

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

Request Money input is not getting focus when navigating back

What is the root cause of that problem?

https://github.com/Expensify/App/blob/882026d659629d47cda410d120a00ac2a2cc0c61/src/components/OptionsSelector/BaseOptionsSelector.js#L122

we just have the logic to focus input in componentDidMount, but if we go to the next page and go back, the previous page won't unmount that why the input is not re-focused

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

In OptionsSelector

We should add the logic to focus the input when the page is visible again in componentDidUpdate like what we did in WorkspaceInviteMessagePage

    componentDidUpdate(prevProps) {
        if (this.props.autoFocus && !prevProps.isFocused && this.props.isFocused) {
            setTimeout(()=>{
                this.textInput.focus();
            },[CONST.ANIMATED_TRANSITION])
        }

Using setTimeout is necessary because we can make sure that it will finish transition and then set focus.

If we don't want to use setTimeout, we can use InteractionManager.runAfterInteractions instead

Result

https://github.com/Expensify/App/assets/113963320/6aa5ba3b-4fe4-4dab-a98e-e3372d36245c

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01612a76428b498945

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

maddylewis commented 1 year ago

assigning contributor to review proposal

multijump commented 1 year ago

Proposal

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

Request Money input is not getting focus when navigating back

What is the root cause of that problem?

When HeaderWithBackButton click, it just go back like browser history back. https://github.com/Expensify/App/blob/777415e3b6a30d40df4bb3c0cfae27860ae3e561/src/libs/Navigation/Navigation.js#L122 Then MoneyRequestParticipantsSelector component didn't rerendered and focus lost in input.

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

In the MoneyRequestConfirm component, fallback url is generated in navigateBack function but it is not used. We can update to use this fallback url forcely to rerender MoneyRequestParticipantsSelector.

    const navigateBack = () => {
        let fallback;
        if (reportID.current) {
            fallback = ROUTES.getMoneyRequestRoute(iouType.current, reportID.current);
        } else {
            fallback = ROUTES.getMoneyRequestParticipantsRoute(iouType.current);
        }
        Navigation.goBack(fallback, true);
    };

We don't need to use setTimeout or change other components.

Result

https://github.com/Expensify/App/assets/35476479/73ee8b95-6c37-4720-8c65-d3718a48c4b9

What alternative solutions did you explore? (Optional)

N/A Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

maddylewis commented 1 year ago

@eVoloshchak - bump on proposal reviews - thx!

eVoloshchak commented 1 year ago

@tienifr's proposal looks good to me I think we should use InteractionManager.runAfterInteractions, the same approach is used on MoneyRequestAmountPage here (except it's relying on useFocusEffect to determine when screen is focused)

🎀👀🎀 C+ reviewed!

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

📣 @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($1000)

melvin-bot[bot] commented 1 year ago

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 1 year ago

📣 @Mahesh! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format: 
    Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
melvin-bot[bot] commented 1 year ago

The BZ member will need to manually hire Mahesh for the Reporter role. Please store your Upwork details and apply to our Upwork job so this process is automatic in the future!

b4s36t4 commented 1 year ago

This bug has been reported by me @cead22 .

tienifr commented 1 year ago

The PR is ready to review!

cc: @eVoloshchak @cead22

cead22 commented 1 year ago

This bug has been reported by me @cead22 .

Thanks for highlighting this @b4s36t4, I think the automation script pulled @mahesh from here. FYI @maddylewis

Issue reported by: @Mahesh-Vagicherla

maddylewis commented 1 year ago

noted! i will manually hire @b4s36t4 as the reporter in Upwork.

luacmartins commented 1 year ago

FYI this issue seems to have the same root cause. Can we fix that as part of the PR linked to this issue?

eVoloshchak commented 1 year ago

@luacmartins, it already is resolved by the PR since the root cause is the same 🎉

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.45-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-08-02. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

maddylewis commented 1 year ago

moving to daily since payments are due early next week

maddylewis commented 1 year ago

Payment due on Aug 2

eVoloshchak commented 1 year ago
eVoloshchak commented 1 year ago

Regression Test Proposal

  1. Click FAB -> Request Money
  2. Enter an amount and click Next
  3. Verify that name & phone number input is focused
  4. Select any of the contacts
  5. Click Back
  6. Verify that name & phone number input is focused

Do we agree 👍 or 👎

maddylewis commented 1 year ago

Payments

b4s36t4 commented 1 year ago

@maddylewis Accepted the offer in upwork. Thanks.

eVoloshchak commented 1 year ago

Requested the payment via NewDot

JmillsExpensify commented 1 year ago

Reviewed details for @eVoloshchak. These details are accurate based on summary from Business Reviewer and are now approved for payment in NewDot.

maddylewis commented 1 year ago

all payments sent - closing!

tienifr commented 10 months ago

I just noticed that this PR is approved by @eVoloshchak within 2 days, without any changes before final approval from Internal engineer and merging, here is the detail:

I think it's eligible for urgency bonus. We also had the same case here

I know this issue was closed so long ago, and I apologize for this inconvenience and appreciate your attention to this matter 🙇. Thank you! cc @maddylewis

tienifr commented 10 months ago

@maddylewis What do you think about this ^?

tienifr commented 9 months ago

@maddylewis friendly bump