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

Fix: list is cut off at bottom in LHN #5891

Merged
merged 2 commits into from
Oct 20, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Oct 15, 2021

Details

Fixed Issues

$ #5005

Tests | QA Steps

  1. Open LHN on IOS.
  2. Check the LHN should not hide behind the bottom home bar.

Please test #3637 as well which will be directly affected by this PR.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS [Affected Platfrom]
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

output_file.mp4

Android

@parasharrajat parasharrajat requested a review from a team as a code owner October 15, 2021 17:18
@MelvinBot MelvinBot requested review from iwiznia and removed request for a team October 15, 2021 17:18
@iwiznia
Copy link
Contributor

iwiznia commented Oct 15, 2021

Looks good, ping me after the tests

@parasharrajat
Copy link
Member Author

@iwiznia I have done my testing. Added video only for IOS as only index.ios.js file is changed.

@parasharrajat
Copy link
Member Author

I just found that the change I have removed has nothing to do with #3637. Clicking on view Details open the Modal Screen which is a screen on Stack navigator. and the Modal component is not used for that at all. It is used for Confirm, Dropdowns, Fab Menu, etc.

@parasharrajat
Copy link
Member Author

@iwiznia I think we can move to the merge.

@parasharrajat parasharrajat changed the title [HOLD] Fix: list is cut off at bottom in LHN Fix: list is cut off at bottom in LHN Oct 20, 2021
@parasharrajat
Copy link
Member Author

@iwiznia Could you please review this?

@parasharrajat
Copy link
Member Author

parasharrajat commented Oct 20, 2021

Gentle Bump... I am getting impatient here. There is another step on my proposal after this PR but we can't test that until this is merged.

@iwiznia

@iwiznia
Copy link
Contributor

iwiznia commented Oct 20, 2021

Sorry I was OOO on monday and worked half day yesterday.

I just found that the change I have removed has nothing to do with #3637.

But allegedly the changes in this PR #3709 which says Fixes #3637 introduced the changes and solved the problem. The issue also says:

incorrectly positioned in some iOS versions

Did you test in the correct iOS versions? (not sure which ones those are). Probably @rdjuric should know

@parasharrajat
Copy link
Member Author

Yeah, I tested on the exact platform which QA has mentioned on the Issue.

But allegedly the changes in this PR #3709 which say Fixes #3637 introduced the changes and solved the problem. The issue also says

let me prove it to you.

The issue reported was coming on the IOU Details Page. and the changes done on that PR were related to BaseModal. BaseModal is used for Confirm Modals and contextMenu, attachment view, etc.

on the other hand, IOUDetails Modal uses React-navigation stack navigator to create the page and it does not utilize the BaseModal in anyways. So I find it hard to believe that changes and issue were related.
cc: @iwiznia

@iwiznia iwiznia merged commit f4c7d98 into Expensify:main Oct 20, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @iwiznia in version: 1.1.8-10 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@parasharrajat parasharrajat deleted the bottomEdgeFix branch November 20, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants