Skip to content
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

Redirect user to concierge when onboarding completed #42087

Merged
merged 14 commits into from
May 30, 2024
Merged

Redirect user to concierge when onboarding completed #42087

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2edff29
redirect user to concierge when onboarding completed
skyweb331 May 13, 2024
efc236a
set onboarding completed as initial
skyweb331 May 14, 2024
3dfb293
remove unused onboarding onyx type
skyweb331 May 15, 2024
3aa75e9
replace withOnyx HOC with useOnyx hook
skyweb331 May 15, 2024
06706c4
fix route change on component rendering
skyweb331 May 15, 2024
9fef64b
navigate to concierge when navigation is ready
skyweb331 May 17, 2024
37aaff4
fix comment for concerige
skyweb331 May 22, 2024
f2758fc
allow array for nvp_onboarding onyx
skyweb331 May 23, 2024
fd3dd47
add comment for nvp_onboarding type
skyweb331 May 23, 2024
bfeb14b
fix prettier
skyweb331 May 27, 2024
b97b952
Merge remote-tracking branch 'upstream/main' into onboarding
skyweb331 May 28, 2024
7a4c242
fix merge error
skyweb331 May 28, 2024
e4a2d44
fix onboarding modal flashing
skyweb331 May 28, 2024
2dd159d
fix return null
skyweb331 May 29, 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
2 changes: 1 addition & 1 deletion src/ONYXKEYS.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ type OnyxValuesMapping = {
[ONYXKEYS.ACCOUNT]: OnyxTypes.Account;
[ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID]: string;
[ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER]: boolean;
[ONYXKEYS.NVP_ONBOARDING]: Onboarding | [];
[ONYXKEYS.NVP_ONBOARDING]: Onboarding;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we change the type? for old accounts the nvp is an empty array so I think we need to keep this

[ONYXKEYS.ACTIVE_CLIENTS]: string[];
[ONYXKEYS.DEVICE_ID]: string;
[ONYXKEYS.IS_SIDEBAR_LOADED]: boolean;
Expand Down
25 changes: 23 additions & 2 deletions src/libs/Navigation/AppNavigator/Navigators/OnboardingModalNavigator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,39 @@
import {createStackNavigator} from '@react-navigation/stack';
import React from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import NoDropZone from '@components/DragAndDrop/NoDropZone';
import useOnboardingLayout from '@hooks/useOnboardingLayout';
import useThemeStyles from '@hooks/useThemeStyles';
import OnboardingModalNavigatorScreenOptions from '@libs/Navigation/AppNavigator/OnboardingModalNavigatorScreenOptions';
import Navigation from '@libs/Navigation/Navigation';
import type {OnboardingModalNavigatorParamList} from '@libs/Navigation/types';
import OnboardingPersonalDetails from '@pages/OnboardingPersonalDetails';
import OnboardingPurpose from '@pages/OnboardingPurpose';
import OnboardingWork from '@pages/OnboardingWork';
import * as Report from '@userActions/Report';
import ONYXKEYS from '@src/ONYXKEYS';
import SCREENS from '@src/SCREENS';
import Overlay from './Overlay';

type OnboardingModalNavigatorProps = {
/** Current onboarding completion status */
hasCompletedGuidedSetupFlow: boolean;
};

const Stack = createStackNavigator<OnboardingModalNavigatorParamList>();

function OnboardingModalNavigator() {
function OnboardingModalNavigator({hasCompletedGuidedSetupFlow}: OnboardingModalNavigatorProps) {
const styles = useThemeStyles();
const {shouldUseNarrowLayout} = useOnboardingLayout();

if (hasCompletedGuidedSetupFlow) {
Navigation.goBack();
Report.navigateToConciergeChat();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Report.navigateToConciergeChat();

I think just going back without redirecting to Concierge is better. Let me know if there are any edge cases in which redirecting to Concierge is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suppose you are in a report and clicked https://dev.new.expensify.com:8082/onboarding link in a message. In this case, if there's only goBack, it will redirect user to onboarding first and then will go back to the current report. So need to redirect user to Concierge again.

What if there's only one Concierge redirect? If so, user will be redirect to concierge on deep link click, but if he clicks go back in the browser, it will redirect user to onbarding and then onboarding will redirect user back to concierge, so user can not go back... so these two actions are required and it will remove onboarding from route history...

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Navigation.goBack();
Report.navigateToConciergeChat();
Navigation.isNavigationReady().then(() => {
Navigation.goBack();
Report.navigateToConciergeChat();
}
navigatorNotReady.mp4

I see the navigator is not ready when navigating to these routes. So, it goes back to onboarding again as seen in the video.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought, this component is defined in Route, so navigation should be ready already...Pushed fix as you mentioned

// eslint-disable-next-line react/jsx-no-useless-fragment
return <></>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// eslint-disable-next-line react/jsx-no-useless-fragment
return <></>;
return null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me, when I test it on IOS and android, return null throws some errors. That's why I put Fragment. Need to test again though.

}

return (
<NoDropZone>
<Overlay />
Expand Down Expand Up @@ -45,4 +61,9 @@ function OnboardingModalNavigator() {

OnboardingModalNavigator.displayName = 'OnboardingModalNavigator';

export default OnboardingModalNavigator;
export default withOnyx<OnboardingModalNavigatorProps, OnboardingModalNavigatorProps>({
Copy link
Contributor

Choose a reason for hiding this comment

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

useOnyx is preferred to withOnyx HOC. Could you change it?

hasCompletedGuidedSetupFlow: {
key: ONYXKEYS.NVP_ONBOARDING,
selector: (onboarding) => onboarding?.hasCompletedGuidedSetupFlow ?? true,
},
})(OnboardingModalNavigator);
2 changes: 2 additions & 0 deletions src/types/onyx/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import type MapboxAccessToken from './MapboxAccessToken';
import type Modal from './Modal';
import type Network from './Network';
import type NewGroupChatDraft from './NewGroupChatDraft';
import type Onboarding from './Onboarding';
import type {OnyxUpdateEvent, OnyxUpdatesFromServer} from './OnyxUpdatesFromServer';
import type {DecisionName, OriginalMessageIOU} from './OriginalMessage';
import type PersonalBankAccount from './PersonalBankAccount';
Expand Down Expand Up @@ -108,6 +109,7 @@ export type {
MapboxAccessToken,
Modal,
Network,
Onboarding,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? I don't see this export from this file being used anywhere.

OnyxUpdateEvent,
OnyxUpdatesFromServer,
PersonalBankAccount,
Expand Down
Loading