Skip to content

Fix keyboard appears briefly when opening concierge page from a chat #37002

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.

Sign up for GitHub

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

Merged
marcochavezf merged 4 commits into from
Feb 28, 2024
Merged

Fix keyboard appears briefly when opening concierge page from a chat #37002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5bcba77
add concierge page to dismiss modal case
bernhardoj Feb 21, 2024
c42e84b
Merge branch 'main' into fix/35239-keyboard-shows-when-open-concierge
bernhardoj Feb 26, 2024
637af8a
remove unused param
bernhardoj Feb 26, 2024
f5a0bd4
fix param
bernhardoj Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libs/Navigation/dismissModal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ function dismissModal(navigationRef: NavigationContainerRef<RootStackParamList>)
case NAVIGATORS.RIGHT_MODAL_NAVIGATOR:
case SCREENS.NOT_FOUND:
case SCREENS.REPORT_ATTACHMENTS:
case SCREENS.CONCIERGE:
navigationRef.dispatch({...StackActions.pop(), target: state.key});
break;
default: {
Expand Down
1 change: 1 addition & 0 deletions src/libs/Navigation/dismissModalWithReport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@bernhardoj bernhardoj Feb 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

@marcochavezf marcochavezf Feb 27, 2024

Copy link
Copy Markdown
Contributor

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?

if (targetReport.reportID !== getTopmostReportId(state)) {
const reportState = getStateFromPath(ROUTES.REPORT_WITH_ID.getRoute(targetReport.reportID));
Expand Down
8 changes: 1 addition & 7 deletions src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1719,7 +1719,7 @@ function updateWriteCapabilityAndNavigate(report: Report, newValue: WriteCapabil
/**
* Navigates to the 1:1 report with Concierge
*/
function navigateToConciergeChat(shouldDismissModal = false, shouldPopCurrentScreen = false, checkIfCurrentPageActive = () => true) {
function navigateToConciergeChat(shouldDismissModal = false, checkIfCurrentPageActive = () => true) {
// If conciergeChatReportID contains a concierge report ID, we navigate to the concierge chat using the stored report ID.
// Otherwise, we would find the concierge chat and navigate to it.
if (!conciergeChatReportID) {
Expand All @@ -1730,17 +1730,11 @@ function navigateToConciergeChat(shouldDismissModal = false, shouldPopCurrentScr
if (!checkIfCurrentPageActive()) {
return;
}
if (shouldPopCurrentScreen && !shouldDismissModal) {
Navigation.goBack();
}

@bernhardoj bernhardoj Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

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.

navigateToAndOpenReport([CONST.EMAIL.CONCIERGE], shouldDismissModal);
});
} else if (shouldDismissModal) {
Navigation.dismissModal(conciergeChatReportID);
} else {
if (shouldPopCurrentScreen) {
Navigation.goBack();
}
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(conciergeChatReportID));
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ConciergePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function ConciergePage({session}: ConciergePageProps) {
if (isUnmounted.current) {
return;
}
Report.navigateToConciergeChat(undefined, true, () => !isUnmounted.current);
Report.navigateToConciergeChat(true, () => !isUnmounted.current);
});
} else {
Navigation.navigate();
Expand Down
2 changes: 1 addition & 1 deletion src/pages/OnboardEngagement/PurposeForUsingExpensifyPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ function PurposeForUsingExpensifyModal() {
}

Report.completeEngagementModal(message, choice);
Report.navigateToConciergeChat(false, true);
Report.navigateToConciergeChat(true);

@bernhardoj bernhardoj Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing the param conflict

@bernhardoj bernhardoj Feb 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 #36283 to solve #36265.

Still works fine.

Screen.Recording.2024-02-26.at.14.05.17.mov

}, []);

const menuItems: MenuItemProps[] = useMemo(
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReimbursementAccount/ConnectBankAccount/ConnectBankAccount.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@bernhardoj bernhardoj Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing the param conflict

@bernhardoj bernhardoj Feb 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

const bankAccountState = reimbursementAccount.achData?.state ?? '';

// If a user tries to navigate directly to the validate page we'll show them the EnableStep
Expand Down