-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: Submit button hang in middle page #51318
Conversation
There is a ESLint error related to useOnyx migration, but I saw we already have a ticket to fix it. |
@truph01 The issue is still there Screen.Recording.2024-10-23.at.10.01.18.PM.movYour recordings didn't follow step 6 and 7. Can you retest on your end please?
|
On mobile safari, select workspace also make the submit button hung in the middle, see recording Screen.Recording.2024-10-23.at.10.13.07.PM.mov |
@truph01 Friendly bump on the above comments! |
@eh2077 Sorry for my delay. I follow the test steps but still could not reproduce the bug when applying my change in Safari: Screen.Recording.2024-10-29.at.15.44.05.movCan you help test again? |
@truph01 Yes, saving description and merchant works now. But selecting a participant still not working. Below are the reproduction steps
mobile-safari.mov |
I am still investigating the bug. |
Hey @truph01, do you have any updates regarding the issue above? |
@eh2077 I am still investigating to find solution for that case. |
Daily update: Still investigating ... |
@eh2077, my proposed solution only works if Screen A has already been rendered (when navigating from A to B, then back to A). However, with the bug we’re addressing here, Screen A has not been rendered yet (navigating from X to A). IMO, I think that bug and the main bug are slightly different. I explored a few options to fix that bug, but they introduced other issues on devices without keyboards. So, I’ve reverted to a solution that uses a setTimeout to show the Submit button after a set delay, like 500ms. What do you think? |
@truph01 I'm just thinking - is it possible to lift up the keyboard opening state to a parent component? |
@eh2077 Thanks, your suggestion works well. I updated PR |
It's EOD for me, so I'll update tmr |
Reviewer Checklist
Screenshots/VideosAndroid: Native0-android.mp4Android: mWeb Chrome0-mobile-chrome.mp4iOS: Native0-ios.mp4iOS: mWeb Safari0-mobile-safari.mp4MacOS: Chrome / SafariN/A, as there's no visual keyboard MacOS: DesktopN/A, as there's no visual keyboard |
[shouldUseNarrowLayout], | ||
); | ||
useEffect(() => { | ||
if (!isWindowHeightReducedByKeyboard && windowHeight < prevWindowHeight - 100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add comments for this effect? Copying from PDFView/index.tsx should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and tested well!
Note: We can apply this improvement to other similar use cases in the app, but I prefer not to be overly aggressive since the issue mainly occurs in mobile Safari. Instead, we can proceed progressively: if there are no regressions after it goes to production, we can create a follow-up PR for a broader rollout.
Yeah, let's first roll this out here and see if it makes it through regression testing |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/thienlnam in version: 9.0.62-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.62-4 🚀
|
Details
Fixed Issues
$ #50663
PROPOSAL: #50663 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-10-23.at.16.39.51.mov
Android: mWeb Chrome
Screen.Recording.2024-10-23.at.16.37.58.mov
iOS: Native
Screen.Recording.2024-10-23.at.16.35.05.mov
iOS: mWeb Safari
Screen.Recording.2024-10-23.at.16.24.14.mov
MacOS: Chrome / Safari
Screen.Recording.2024-10-23.at.16.30.57.mov
MacOS: Desktop
Screen.Recording.2024-10-23.at.16.37.11.mov