-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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: 10731 Focus composer without showing keyboard when users go to chats #32711
base: main
Are you sure you want to change the base?
fix: 10731 Focus composer without showing keyboard when users go to chats #32711
Conversation
@Santhosh-Sellavel Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
I am not sure if this is the expected behavior. @ntdiary can you confirm this doesn't conflict with your refactor? |
|
I need some time to understand this issue first. :) |
@ntdiary @0xmiroslav bump
|
@Santhosh-Sellavel, we have a PR #29199 that aims to standardize the behavior of refocusing when a modal is closed (including So, after it is merged, the main input box will no longer gain focus if we complete the money request from the green plus button, which means that the keyboard will no longer be pulled up (for all mobile native/mWeb platforms). As for whether the main composer should be focused when we open a chat from LHN, it is beyond the scope of that PR. demo.mp4 |
@christianwen can you resolve conflicts, please? |
merged main |
@christianwen Sorry for the delay, can resolve conflicts please? |
@Santhosh-Sellavel I fixed the conflicts, can you help process review this PR to avoid the conflicts? Thanks |
@christianwen Screen.Recording.2024-01-26.at.1.04.52.AM.mov |
Checking... |
Any update? |
Bump @christianwen |
I faced with some errors when building iOS.Pls give me a day to fix it |
@christianwen this happens on mWeb iOS |
Any update @christianwen |
I'll raise the fix on rn-live-markdown soon |
@christianwen Please ping me when all dones |
@DylanDylann PR fix on RN-livemarkdown: Expensify/react-native-live-markdown#499 |
I also cleanup isFocusedWhileChangingInputMode logic, maybe it's fixed somewhere else |
@christianwen Please upgrade the RN live markdown version |
@christianwen Also please resolve conflict and failure test |
@DylanDylann Updated! For the perf issue, we shouldn't mind about it since this test is to compare the number of re-render when users interact with input in baseline (from main) with the current behavior (this PR). In this PR we try to open the keyboard when they focus on the composer -> it takes 2 times to re-render |
@christianwen You mean this perf test is invalid now |
How about this comment: https://github.com/Expensify/App/pull/32711/files#r1758443883 |
will update soon |
@christianwen Bump on #10731 (comment) |
Thank you @DylanDylann. I'm updating the PR |
@DylanDylann updated! |
Yes |
@christianwen If that, should we update the perf test to align with new change |
I think we can't, since we're comparing the behavior between main and this PR |
Ahh see. Thanks |
@christianwen Safari Bug: Keyboard doesn't appear Screen.Recording.2024-09-25.at.15.28.07.mov |
BUG: The keyboard doesn't show on Android (only show if clicking two times) Screen.Recording.2024-09-25.at.17.44.01.mov |
Details
Fixed Issues
$ #10731
PROPOSAL: #10731 (comment)
Tests
Offline tests
Same as above
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 methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
e-android.native.mov
Android: mWeb Chrome
e-android.mweb.mov
iOS: Native
e-ios.mov
iOS: mWeb Safari
e-safari.mov
MacOS: Chrome / Safari
e-web.mov
MacOS: Desktop
e-desktop.mov