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.

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

Refactor bookATrip function #55852

Merged
merged 29 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d18f99b
Pass policy to Travel.bookATrip
cristipaval Jan 28, 2025
9ae5e55
Remove activePolicyID from Travel.ts
cristipaval Jan 28, 2025
1a32608
Remove primaryLogin from Travel.ts
cristipaval Jan 28, 2025
bc569f5
Add footer to the FeatureList component
cristipaval Jan 28, 2025
d16741c
Move Travel specific components out of FeatureList
cristipaval Jan 28, 2025
70d8109
Move bookATrip logic to BookTravelButton
cristipaval Jan 28, 2025
358fe95
Get primaryLogin from Onyx
cristipaval Jan 28, 2025
789a92b
Add body prop instead of reusing subtitle for everything
cristipaval Jan 28, 2025
fa0ddbb
Put BookTravelButton in the body of the EmptyStateComponent
cristipaval Jan 28, 2025
22f3617
Removed unused bookATrip function from Travel.ts
cristipaval Jan 28, 2025
c6e30de
Remove antipattern provisionDomain function
cristipaval Jan 28, 2025
b633ba0
Remove antipattern handleProvisioningPermissionDeniedError function
cristipaval Jan 28, 2025
00d6012
Remove antipattern openTravelDotAfterProvisioning function
cristipaval Jan 28, 2025
243a09e
Use body instead of subtitle
cristipaval Jan 28, 2025
46b6485
Make ESLint happt
cristipaval Jan 28, 2025
f9428d6
Add comments
cristipaval Jan 30, 2025
f10ee2e
Move function out of the tsx
cristipaval Jan 30, 2025
cf3e4bd
Add comment
cristipaval Jan 30, 2025
2331d13
New function for navigating to Accept Terms
cristipaval Feb 3, 2025
ab8d5db
Add comments
cristipaval Feb 3, 2025
7dd2c55
children instead of body in EmptyStateComponentProps
cristipaval Feb 3, 2025
f8e248a
Move function in the component since it has a dependency
cristipaval Feb 4, 2025
14ee3bc
Merge remote-tracking branch 'origin/main' into cristi_fix-bookATrip-…
cristipaval Feb 4, 2025
950ee37
Disable root status bar after going back to classic
cristipaval Feb 4, 2025
75e4342
Fix ESLint
cristipaval Feb 4, 2025
e43921a
Update src/components/BookTravelButton.tsx
cristipaval Feb 4, 2025
820a0e5
Update src/components/EmptyStateComponent/index.tsx
cristipaval Feb 4, 2025
f7f58db
Tiny improvements suggested by reviewers
cristipaval Feb 4, 2025
b851e2f
Update src/components/BookTravelButton.tsx
cristipaval Feb 4, 2025
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
31 changes: 19 additions & 12 deletions src/components/BookTravelButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ type BookTravelButtonProps = {
text: string;
};

const navigateToAcceptTerms = (domain: string) => {
// Remove the previous provision session infromation if any is cached.
cleanupTravelProvisioningSession();
Navigation.navigate(ROUTES.TRAVEL_TCS.getRoute(domain));
};

Comment on lines +25 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor comment
But similar case as here
We usually don't move such functions out of the component 😅

#55852 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, we want it outside of the component since it has no dependency on the component, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right
As I mentioned, it's just a minor comment
Also, keeping the function outside a component prevents it from being recreated on every render
What I meant is that we usually keep wrapper functions inside the component
But I don't mind leaving it as is !

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it's OK where it's at, and I also agree that I kinda wish more pure functions like this were moved outside of the component entirely. It's not seen very often in the app, but should be OK here.

function BookTravelButton({text}: BookTravelButtonProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
Expand All @@ -36,11 +42,15 @@ function BookTravelButton({text}: BookTravelButtonProps) {
const [isSingleNewDotEntry] = useOnyx(ONYXKEYS.IS_SINGLE_NEW_DOT_ENTRY);

const bookATrip = useCallback(() => {
setErrorMessage('');

// The primary login of the user is where Spotnana sends the emails with booking confirmations, itinerary etc. It can't be a phone number.
if (!primaryLogin || Str.isSMSLogin(primaryLogin)) {
setErrorMessage(translate('travel.phoneError'));
return;
}

// An address is required when the Spotnana entity is created for the policy
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is an address required? (answer that in the comment please)

if (isEmptyObject(policy?.address)) {
Navigation.navigate(ROUTES.WORKSPACE_PROFILE_ADDRESS.getRoute(policy?.id, Navigation.getActiveRoute()));
return;
Expand All @@ -63,26 +73,23 @@ function BookTravelButton({text}: BookTravelButtonProps) {
?.catch(() => {
setErrorMessage(translate('travel.errorMessage'));
});
if (errorMessage) {
setErrorMessage('');
}
} else if (isPolicyProvisioned) {
cleanupTravelProvisioningSession();
Navigation.navigate(ROUTES.TRAVEL_TCS.getRoute(CONST.TRAVEL.DEFAULT_DOMAIN));
navigateToAcceptTerms(CONST.TRAVEL.DEFAULT_DOMAIN);
} else {
// Determine the domain to associate with the workspace during provisioning in Spotnana.
// - If all admins share the same private domain, the workspace is tied to it automatically.
// - If admins have multiple private domains, the user must select one.
// - Public domains are not allowed; an error page is shown in that case.
const adminDomains = getAdminsPrivateEmailDomains(policy);
let routeToNavigateTo;
if (adminDomains.length === 0) {
routeToNavigateTo = ROUTES.TRAVEL_PUBLIC_DOMAIN_ERROR;
Navigation.navigate(ROUTES.TRAVEL_PUBLIC_DOMAIN_ERROR);
} else if (adminDomains.length === 1) {
cleanupTravelProvisioningSession();
routeToNavigateTo = ROUTES.TRAVEL_TCS.getRoute(adminDomains.at(0) ?? CONST.TRAVEL.DEFAULT_DOMAIN);
navigateToAcceptTerms(adminDomains.at(0) ?? CONST.TRAVEL.DEFAULT_DOMAIN);
} else {
routeToNavigateTo = ROUTES.TRAVEL_DOMAIN_SELECTOR;
Navigation.navigate(ROUTES.TRAVEL_DOMAIN_SELECTOR);
}
Navigation.navigate(routeToNavigateTo);
}
}, [policy, isSingleNewDotEntry, travelSettings, translate, errorMessage, primaryLogin]);
}, [policy, isSingleNewDotEntry, travelSettings, translate, primaryLogin]);

return (
<>
Expand Down
4 changes: 2 additions & 2 deletions src/components/EmptyStateComponent/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function EmptyStateComponent({
title,
titleStyles,
subtitle,
body,
children,
headerStyles,
headerContentStyles,
lottieWebViewStyles,
Expand Down Expand Up @@ -101,7 +101,7 @@ function EmptyStateComponent({
<View style={shouldUseNarrowLayout ? styles.p5 : styles.p8}>
<Text style={[styles.textAlignCenter, styles.textHeadlineH1, styles.mb2, titleStyles]}>{title}</Text>
<Text style={[styles.textAlignCenter, styles.textSupporting, styles.textNormal]}>{subtitle}</Text>
{!!body && body}
{!!children && children}
<View style={[styles.gap2, styles.mt5, !shouldUseNarrowLayout ? styles.flexRow : undefined]}>
{buttons?.map(({buttonText, buttonAction, success, icon, isDisabled}, index) => (
<View
Expand Down
2 changes: 1 addition & 1 deletion src/components/EmptyStateComponent/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type SharedProps<T> = {
title: string;
titleStyles?: StyleProp<TextStyle>;
subtitle?: string;
body?: React.ReactNode;
children?: React.ReactNode;
buttons?: Button[];
containerStyles?: StyleProp<ViewStyle>;
headerStyles?: StyleProp<ViewStyle>;
Expand Down
11 changes: 6 additions & 5 deletions src/pages/Search/EmptySearchView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ function EmptySearchView({type, hasResults}: EmptySearchViewProps) {
return areAllGroupPoliciesExpenseChatDisabled((allPolicies as OnyxCollection<Policy>) ?? {});
}, [allPolicies]);

const contentBody = useMemo(() => {
const contentChildren = useMemo(() => {
Copy link
Contributor

@ZhenjaHorbach ZhenjaHorbach Feb 4, 2025

Choose a reason for hiding this comment

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

Maybe we should rename this const since this is a bit confusing because we also have content.children in this component ?

Maybe we should rename it to tripChildren since this children relevant only for CONST.SEARCH.DATA_TYPES.TRIP ?

return (
<>
<Text style={[styles.textSupporting, styles.textNormal]}>
Expand Down Expand Up @@ -120,7 +120,7 @@ function EmptySearchView({type, hasResults}: EmptySearchViewProps) {
headerContentStyles: [StyleUtils.getWidthAndHeightStyle(375, 240), StyleUtils.getBackgroundColorStyle(theme.travelBG)],
title: translate('travel.title'),
titleStyles: {...styles.textAlignLeft},
body: contentBody,
children: contentChildren,
lottieWebViewStyles: {backgroundColor: theme.travelBG, ...styles.emptyStateFolderWebStyles},
};
case CONST.SEARCH.DATA_TYPES.EXPENSE:
Expand Down Expand Up @@ -220,7 +220,7 @@ function EmptySearchView({type, hasResults}: EmptySearchViewProps) {
translate,
styles.textAlignLeft,
styles.emptyStateFolderWebStyles,
contentBody,
contentChildren,
hasSeenTour,
navatticURL,
shouldRedirectToExpensifyClassic,
Expand All @@ -241,11 +241,12 @@ function EmptySearchView({type, hasResults}: EmptySearchViewProps) {
title={content.title}
titleStyles={content.titleStyles}
subtitle={content.subtitle}
body={content.body}
buttons={content.buttons}
headerContentStyles={[styles.h100, styles.w100, ...content.headerContentStyles]}
lottieWebViewStyles={content.lottieWebViewStyles}
/>
>
{content.children}
</EmptyStateComponent>
<ConfirmModal
prompt={translate('sidebarScreen.redirectToExpensifyClassicModal.description')}
isVisible={modalVisible}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,6 @@ function WorkspaceCompanyCardsFeedPendingPage() {
const {translate} = useLocalize();
const styles = useThemeStyles();

const body = (
<Text>
{translate('workspace.moreFeatures.companyCards.pendingFeedDescription')}
<TextLink onPress={() => navigateToConciergeChat()}> {CONST?.CONCIERGE_CHAT_NAME}</TextLink>.
</Text>
);

return (
<EmptyStateComponent
SkeletonComponent={CardRowSkeleton}
Expand All @@ -30,8 +23,12 @@ function WorkspaceCompanyCardsFeedPendingPage() {
headerStyles={[styles.emptyStateCardIllustrationContainer, {backgroundColor: colors.ice800}]}
headerContentStyles={styles.pendingStateCardIllustration}
title={translate('workspace.moreFeatures.companyCards.pendingFeedTitle')}
body={body}
/>
>
<Text>
{translate('workspace.moreFeatures.companyCards.pendingFeedDescription')}
<TextLink onPress={() => navigateToConciergeChat()}> {CONST?.CONCIERGE_CHAT_NAME}</TextLink>.
Copy link
Contributor

@ZhenjaHorbach ZhenjaHorbach Feb 4, 2025

Choose a reason for hiding this comment

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

I think ? is redundant for CONSTs

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

</Text>
</EmptyStateComponent>
);
}

Expand Down