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.5k stars 2.85k forks source link

[$250] Open duplicate expense - “This expense is on Hold” content overlaps with the keyboard. #51060

Open m-natarajan opened 1 week ago

m-natarajan commented 1 week 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: 9.0.50-5 Reproducible in staging?: y Reproducible in production?: y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: 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 Expensify/Expensify Issue URL: Issue reported by: @suneox Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1729092611052969

Action Performed:

Precondition:

Actual Result:

The modal introducing “This expense is on Hold” opens and the content overlaps with the keyboard.

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence https://github.com/user-attachments/assets/5bd2f6a5-e65a-4fd5-94a8-b5d01c37c8e6

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021849438927318784165
  • Upwork Job ID: 1849438927318784165
  • Last Price Increase: 2024-10-24
Issue OwnerCurrent Issue Owner: @paultsimura
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @muttmuure (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.

suneox commented 1 week ago

@muttmuure I think this issue can make for external. I have more context on it so I can assist with reviewing the proposal as a C+.

https://github.com/user-attachments/assets/635e4b6a-8d73-4b7b-9562-ac340599b258

NJ-2020 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-10-18 08:13:16 UTC.

Proposal

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

Open duplicate expense - “This expense is on Hold” content overlaps with the keyboard

What is the root cause of that problem?

We're not dismissing the keyboard when navigation to PROCESS_MONEY_REQUEST_HOLD page https://github.com/Expensify/App/blob/e4fe4c4fe126db86cd3ecb8d918d81d14ac09942/src/components/MoneyReportHeader.tsx#L302-L305 And same here https://github.com/Expensify/App/blob/e4fe4c4fe126db86cd3ecb8d918d81d14ac09942/src/components/MoneyRequestHeader.tsx#L133-L136

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

We should dismiss the keyboard before navigating

if (isSmallScreenWidth) {
    if (Navigation.getActiveRoute().slice(1) === ROUTES.PROCESS_MONEY_REQUEST_HOLD.route) {
        Keyboard.dismiss();
        Navigation.goBack();
    }
} else {
    ...

What alternative solutions did you explore? (Optional)

We can dismiss the keyboard outside the if statement so the Keyboard.dismiss() will get invoked both in small screen and large screen in case if the isSmallScreenWidth value is undefined

Keyboard.dismiss();
if (isSmallScreenWidth) {
    if (Navigation.getActiveRoute().slice(1) === ROUTES.PROCESS_MONEY_REQUEST_HOLD.route) {
        Navigation.goBack();
    }
} else {
    ...
melvin-bot[bot] commented 6 days ago

@muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 4 days ago

@muttmuure Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 3 days ago

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

melvin-bot[bot] commented 3 days ago

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

paultsimura commented 3 days ago

Reviewing soon 👀

FitseTLT commented 2 days ago

Proposal

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

mWeb - Dupe detect - Keyboard displayed below educational modal when open dupe expense

What is the root cause of that problem?

The composer auto focuses on opening the transaction thread and then popover will be open as soon as shouldShowHoldMenu is set to true here https://github.com/Expensify/App/blob/f46bce0b96c533b6aac0e2464ae27615101a7f7d/src/components/MoneyRequestHeader.tsx#L127 so the popover will overlap with the keyboard

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

We should blur the report action compose whenever we display the hold menu (isVisible changes to true) Here https://github.com/Expensify/App/blob/f46bce0b96c533b6aac0e2464ae27615101a7f7d/src/components/ProcessMoneyRequestHoldMenu.tsx#L44

useEffect(() => {
        if (!isVisible) {
            return;
        }
        ReportActionComposeFocusManager.composerRef.current?.blur();
    }, [isVisible]);

FYI: Keyboard.dismiss doesn't suffice as the cursor will be visible under the popover and most importantly it doesn't work for web

What alternative solutions did you explore? (Optional)

We can also use Focus Trap in ProcessMoneyRequestHoldMenu (or generally in Popover component) by passing isVisible to active prop, that will automatically defocus the compose or any other text input when the popover is displayed

FitseTLT commented 2 days ago

@paultsimura this is a dupe of https://github.com/Expensify/App/issues/51398#issuecomment-2435320794 so reposting my proposal here because this one is older. 👍

paultsimura commented 5 hours ago

Thanks for the proposals.

@FitseTLT could you please share the recording of this case?

the cursor will be visible under the popover

Also, you say that it doesn't work for web – but the issue is not relevant for the web, is it?