Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS - IOU - Save button and offline message in Edit Amount page overlaps with navigation bar #30840

Closed
1 of 6 tasks
kbecciv opened this issue Nov 3, 2023 · 14 comments
Closed
1 of 6 tasks
Assignees
Labels
Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Nov 3, 2023

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.3.95.0
Reproducible in staging?: y
Reproducible in production?: n
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: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Launch New Expensify app.
  2. Tap + > Request money.
  3. Create a manual request.
  4. Open the IOU details view.
  5. Tap Amount.
  6. Return to IOU details view.
  7. Go offline.
  8. Tap Amount.

Expected Result:

In Step 5, the Save button will not overlap with the white navigation bar.
In Step 8, offline message will not overlap with the white navigation bar.

Actual Result:

In Step 5, the Save button overlaps with the white navigation bar briefly.
In Step 8, the offline message overlaps with the white navigation bar.

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6262201_1699014823888.RPReplay_Final1699014591.mp4

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Nov 3, 2023
@OSBotify
Copy link
Contributor

OSBotify commented Nov 3, 2023

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Nov 3, 2023

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

@ishpaul777
Copy link
Contributor

ishpaul777 commented Nov 3, 2023

Problem

Save button and offline message in Edit Amount page overlaps with navigation bar

This Looks like more of a inconsistency we dont have a offline indicator in request flow Amount page but we have one in edit amount page. 🤔

orginal Request Amount page edit Request Amount page
Screenshot 2023-11-03 at 6 37 55 PM Screenshot 2023-11-03 at 6 39 28 PM

Root Cause:

In NewRequestAmountPage we are passing shouldEnableKeyboardAvoidingView={false} which disables the KeyboardAvoidingView and we and offline indicator is not visible as KeyboardAvoidingView contains the offline indicator and it is disabled. As KeyboardAvoidingView is only a ios coponent( we use normal View in others) we only notice this in ios app

in EditRequestAmountPage we pass not such prop which disables the KeyboardAvoidingView so the offline indicator is visible.

why the indicator is overlapped with home button?
we implemented the minheight for ScreenWrapper in this PR #30065, so the full inital height of the screen is applied to KeyboardAvoidingView

Changes

Exact changes will depend on whether we want the offline indicator in amount page bot edit and Original and on all platforms or not.

In case we want not show the indicator we will use shouldShowOfflineIndicator and set false in ScreenWrapper in NewRequestAmountPage and EditRequestAmountPage

if we do want the indicator we have refactor the layout and have to move out the offline indicator, also apply necessary padding style, we can introduce a new prop shouldShowOfflineIndicatorOutsideAvoidingView for the this case.

@s-alves10
Copy link
Contributor

This looks like a regression of this PR

This PR introduces the shouldEnableMinHeight and apply it to EditRequestAmountPage and MoneyRequestSelectorPage

shouldEnableMinHeight={DeviceCapabilities.canUseTouchScreen()}

shouldEnableMinHeight={DeviceCapabilities.canUseTouchScreen()}

So this issue happens in MoneyRequestSelector page as well(in offline mode)

In order to solve this issue, we need to minHeight considering the paddingBottom and paddingTop

  1. Remove this line
    const minHeight = shouldEnableMinHeight ? initialHeight : undefined;
  2. Add the following code here
        let minHeight = shouldEnableMinHeight ? initialHeight - (paddingStyle.paddingBottom || 0) - (paddingStyle.paddingTop || 0) : undefined;

This works as expected.

@fedirjh
Copy link
Contributor

fedirjh commented Nov 3, 2023

cc @sarious, could you handle this regression?

@sarious
Copy link
Contributor

sarious commented Nov 4, 2023

cc @sarious, could you handle this regression?

Yes, sure, it seems I found the issue, but now I can't the check it on android/ios browser, because the flow of running the app has changed, because of certificates.

@s-alves10
Copy link
Contributor

@sarious

What do you think is the root cause of the issue?

@sarious
Copy link
Contributor

sarious commented Nov 4, 2023

cc @sarious, could you handle this regression?

@fedirjh should I create a new Pull Request to fix this issue? or where is it better to describe the fix?

@fedirjh
Copy link
Contributor

fedirjh commented Nov 4, 2023

should I create a new Pull Request to fix this issue?

@sarious Yes, you should create a new PR and include links to both this issue and the original issue.

Edit: Please be aware that any issues labeled as DeployBlockerCash need to be addressed promptly and resolved as soon as possible.

@sarious
Copy link
Contributor

sarious commented Nov 4, 2023

should I create a new Pull Request to fix this issue?

@sarious Yes, you should create a new PR and include links to both this issue and the original issue.

Edit: Please be aware that any issues labeled as DeployBlockerCash need to be addressed promptly and resolved as soon as possible.

I understand that, so I've spent today the whole day to fix the issue. Of course, it took a lot of time to launch the application on different devices, due to the fact that the launch process changed, due to the addition of certificates. I still couldn't launch the app in Chrome on Android.

@ishpaul777
Copy link
Contributor

i can help May i know what you need to test I have the android chrome running locally

@sarious
Copy link
Contributor

sarious commented Nov 4, 2023

@ishpaul777 so, I've created a Pull Request with the fix. So I would be very grateful if you can watch it on Android mWeb Chrome and record the video. I will keep trying to run this on my emulator

@sarious
Copy link
Contributor

sarious commented Nov 4, 2023

@ishpaul777 I've done it by myself, but thank you for proposal.

@puneetlath puneetlath removed the DeployBlockerCash This issue or pull request should block deployment label Nov 6, 2023
@puneetlath
Copy link
Contributor

Ok I'm going to close this since the fix has been CP'd and there's no payment needed specifically for this issue. Thanks y'all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants