-
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/10731 auto focus input #51172
base: main
Are you sure you want to change the base?
Fix/10731 auto focus input #51172
Conversation
Updated change compare to previous PR: For 50683: The PR works well without any change, maybe after moving to chat search modal, it's fixed For 50722: This issue happens even on main, you can see the flicker when the keyboard open. It's the bug from |
@DylanDylann 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] |
Reviewing today |
if (!textInputRef) { | ||
throw new Error('textInputRef is not available. This should never happen and indicates a developer error.'); | ||
} | ||
useImperativeHandle( |
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.
@christianwen Is there any new update on this file?
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.
no new update, just moved to the new file
@christianwen Could you also please help to merge the latest main? |
@DylanDylann I merged main and resolved conflicts |
@christianwen It is a bit flicker on Safari Screen.Recording.2024-11-13.at.18.35.39.mov |
I removed in App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx Line 282 in d5e7a5a
since we want to enable autoFocus in every devices. but it breaks the following logic App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx Line 646 in d5e7a5a
Based on this comment App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx Lines 643 to 645 in d5e7a5a
we need to add |
@christianwen BUG on safari: when going to the chat, the cursor displays in the middle or in the end of the composer in a moment
Screen.Recording.2024-11-18.at.16.39.33.mov |
The cursor is hidden when the transition is not finished on staging Screen.Recording.2024-11-20.at.15.56.44.movBut when the inputMode is none the cursor is not hidden during report transition. I spent time to invest, unfortunately, I didn't find the RCA, so I just hide the cursor same as what happens on staging. |
@christianwen With your solution, the draft message will not display right after going to the chat Screen.Recording.2024-11-20.at.17.44.38.mov |
@DylanDylann I fixed the issue above. Pls take a look |
@christianwen BUG on mobile web
On staging: The main composer isn't getting focused again Screen.Recording.2024-11-26.at.10.24.40.mov |
BUG on IOS: The composer is hidden by the keyboard Behavior on this change Screen.Recording.2024-11-26.at.11.17.32.movBehavior on the main Screen.Recording.2024-11-26.at.11.16.59.mov |
@DylanDylann I fixed this issue. For this bug, the RCA is because on small screen, we hide the main composer when focus on edit composer. When users click outside to blur, the main composer will appear and trigger the logic autoFocus (this value is true). I know it's quite different from staging/main but I don't see any problems about this behavior so we can ignore it for now. Pls let me know what you think. |
But the cursor is moving to the beginning of the main composer and I think it is incorrect. |
@DylanDylann I can't reproduce your bug above? |
@christianwen Please try more time Screen.Recording.2024-12-05.at.17.25.00.mov |
Flicker on IOS
Screen.Recording.2024-12-05.at.17.40.06.mov |
@christianwen Kindly bump |
Details
Fixed Issues
$ #10731
PROPOSAL: #10731 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above
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-31.at.11.47.30.mov
Android: mWeb Chrome
Screen.Recording.2024-10-31.at.11.43.17.mov
iOS: Native
Screen.Recording.2024-10-31.at.11.49.52.mov
iOS: mWeb Safari
Screen.Recording.2024-10-31.at.11.39.29.mov
MacOS: Chrome / Safari
Screen.Recording.2024-10-31.at.11.39.54.mov
MacOS: Desktop
Screen.Recording.2024-10-31.at.11.53.43.mov