-
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 keyboard appears briefly when opening concierge page from a chat #37002
Fix keyboard appears briefly when opening concierge page from a chat #37002
Conversation
@shubham1206agra 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] |
@@ -38,6 +38,7 @@ function dismissModalWithReport(targetReport: Report | EmptyObject, navigationRe | |||
case NAVIGATORS.RIGHT_MODAL_NAVIGATOR: | |||
case SCREENS.NOT_FOUND: | |||
case SCREENS.REPORT_ATTACHMENTS: | |||
case SCREENS.CONCIERGE: | |||
// If we are not in the target report, we need to navigate to it after dismissing the modal |
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 decided to use the alternative solution. Here is why:
I have another PR here that is related to the concierge page too. When working on the PR, I tried to smoothen the navigation by replacing GO BACK with REPLACE, just like what we want to do here, but found out that it can open multiple consecutive concierge chats.
For example, when you have chat A and concierge chat.
[A, Concierge Chat]
Then, you open another concierge page.
[A, Concierge Chat, Concierge Page]
If we just use the REPLACE, we will have 2 concierge chats.
[A, Concierge Chat, Concierge Chat]
But if we use dismissModal
, it will check first whether the topmost report is the same as the target report. If it's the same, it will just pop the concierge page out, otherwise, it will replace the concierge page with the concierge chat.
This will conflict with the PR I linked above, so we can wait a bit.
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.
This will conflict with the PR I linked above, so we can wait a bit.
Are you suggesting waiting for the other PR to be merged and deployed? Or do you think it's safe now to merge these changes now that the other PR has been merged?
@bernhardoj Just fix conflicts and we can proceed here. |
@@ -1730,17 +1730,11 @@ function navigateToConciergeChat(shouldDismissModal = false, shouldPopCurrentScr | |||
if (!checkIfCurrentPageActive()) { | |||
return; | |||
} | |||
if (shouldPopCurrentScreen && !shouldDismissModal) { | |||
Navigation.goBack(); | |||
} |
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.
Why I remove this? It's related to my comment above. I added this from the linked PR above. This is only being used in ConciergePage, but since we decided to use dismiss modal, this param is not needed anymore.
@@ -90,7 +90,7 @@ function PurposeForUsingExpensifyModal() { | |||
} | |||
|
|||
Report.completeEngagementModal(message, choice); | |||
Report.navigateToConciergeChat(false, true); | |||
Report.navigateToConciergeChat(true); |
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.
Fixing the param conflict
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.
@@ -39,7 +39,7 @@ function ConnectBankAccount({reimbursementAccount, onBackButtonPress, account, p | |||
const styles = useThemeStyles(); | |||
const {translate} = useLocalize(); | |||
|
|||
const handleNavigateToConciergeChat = () => Report.navigateToConciergeChat(false, true); | |||
const handleNavigateToConciergeChat = () => Report.navigateToConciergeChat(true); |
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.
Fixing the param conflict
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.
The param to dismiss the modal was added in bank account refactor.
Still works fine
Screen.Recording.2024-02-26.at.14.09.47.mov
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-02-27.at.2.11.29.PM.movAndroid: mWeb ChromeScreen.Recording.2024-02-27.at.1.16.08.PM.moviOS: NativeScreen.Recording.2024-02-27.at.1.23.23.PM.moviOS: mWeb SafariScreen.Recording.2024-02-27.at.12.48.31.PM.movMacOS: Chrome / SafariNA MacOS: DesktopNA |
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, just waiting to confirm if we still need to wait for the other PR to makes changes here
@@ -38,6 +38,7 @@ function dismissModalWithReport(targetReport: Report | EmptyObject, navigationRe | |||
case NAVIGATORS.RIGHT_MODAL_NAVIGATOR: | |||
case SCREENS.NOT_FOUND: | |||
case SCREENS.REPORT_ATTACHMENTS: | |||
case SCREENS.CONCIERGE: | |||
// If we are not in the target report, we need to navigate to it after dismissing the modal |
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.
This will conflict with the PR I linked above, so we can wait a bit.
Are you suggesting waiting for the other PR to be merged and deployed? Or do you think it's safe now to merge these changes now that the other PR has been merged?
@marcochavezf the other PR was merged and I already fixed the conflict. |
✋ 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/marcochavezf in version: 1.4.45-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.45-6 🚀
|
Details
The keyboard appears when the concierge page utility is closed before navigating to the real concierge chat, triggering composer refocus logic.
Fixed Issues
$ #35239
PROPOSAL: #35239 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
staging.new.expensify.com/concierge
to the chatPR 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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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-02-21.at.15.52.17.mov
Android: mWeb Chrome
Screen.Recording.2024-02-21.at.15.38.53.mov
iOS: Native
Screen.Recording.2024-02-21.at.15.41.48.mov
iOS: mWeb Safari
Screen.Recording.2024-02-21.at.15.39.44.mov
MacOS: Chrome / Safari
MacOS: Desktop