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

Simplify the RootNavigator structure #42582

Merged
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
fb87e2e
Move settings to AuthScreens
WojtekBoman May 22, 2024
1411be4
Remove CentralPane
WojtekBoman May 22, 2024
aa779db
Add adjustments for new nav structure
WojtekBoman May 23, 2024
87ccd3e
Add CENTRAL_PANE_SCREENS
WojtekBoman May 24, 2024
a381924
Rename CentralPaneScreen to CentralPaneName
WojtekBoman May 24, 2024
da6c4fb
Adjust switchPolicyID to new nav structure
WojtekBoman May 24, 2024
43d4ca0
Remove getTopmostRoute
WojtekBoman May 24, 2024
7eef033
Add withPrepareCentralPaneScreen
WojtekBoman May 28, 2024
5410620
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 3, 2024
eb2473d
Add ts fixes
WojtekBoman Jun 3, 2024
118c049
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 3, 2024
e87c30a
AuthScreens and CENTRAL_PANE_SCREENS cleanup
WojtekBoman Jun 3, 2024
ca681c7
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 4, 2024
c06c5dc
Fix storybook tests
WojtekBoman Jun 4, 2024
09ba271
Add docs for withPrepareCentralPaneScreen
WojtekBoman Jun 4, 2024
28c7a3f
Rename CentralPaneNameOptions
WojtekBoman Jun 4, 2024
99d5523
fix tests
adamgrzybowski Jun 5, 2024
a7a288a
Merge branch 'main' into nav/flatten-central-pane
adamgrzybowski Jun 5, 2024
64d29a7
Merge branch 'main' into nav/flatten-central-pane
adamgrzybowski Jun 7, 2024
19e05dd
fix types
adamgrzybowski Jun 7, 2024
3fc2ede
Merge branch 'main' into nav/flatten-central-pane
adamgrzybowski Jun 7, 2024
5a133a7
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 10, 2024
9bc1bec
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 12, 2024
be4d349
Fix types for central pane screens
WojtekBoman Jun 12, 2024
baaf214
fix central pane blinking on android
adamgrzybowski Jun 12, 2024
e92622a
fix accessing route params object in report screen
adamgrzybowski Jun 13, 2024
0698b3f
Merge branch 'main' into nav/flatten-central-pane
adamgrzybowski Jun 14, 2024
73721e3
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 19, 2024
9d137bd
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 21, 2024
70254e5
Cleaunp flatten central pane code
WojtekBoman Jun 21, 2024
c905a4c
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 24, 2024
38c21a0
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 25, 2024
0efb9f0
Merge branch 'main' into nav/flatten-central-pane
WojtekBoman Jun 25, 2024
becff51
Fix lint in switchPolicyID
WojtekBoman Jun 25, 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
4 changes: 2 additions & 2 deletions src/ROUTES.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type {ValueOf} from 'type-fest';
import type CONST from './CONST';
import type {IOUAction, IOUType} from './CONST';
import type {IOURequestType} from './libs/actions/IOU';
import type {CentralPaneNavigatorParamList} from './libs/Navigation/types';
import type {AuthScreensParamList} from './libs/Navigation/types';
import type {SearchQuery} from './types/onyx/SearchResults';
import type AssertTypesNotEqual from './types/utils/AssertTypesNotEqual';

Expand Down Expand Up @@ -37,7 +37,7 @@ const ROUTES = {

SEARCH: {
route: '/search/:query',
getRoute: (searchQuery: SearchQuery, queryParams?: CentralPaneNavigatorParamList['Search_Central_Pane']) => {
getRoute: (searchQuery: SearchQuery, queryParams?: AuthScreensParamList['Search_Central_Pane']) => {
const {sortBy, sortOrder} = queryParams ?? {};

if (!sortBy && !sortOrder) {
Expand Down
4 changes: 2 additions & 2 deletions src/components/ScreenWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import useTackInputFocus from '@hooks/useTackInputFocus';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as Browser from '@libs/Browser';
import type {CentralPaneNavigatorParamList, RootStackParamList} from '@libs/Navigation/types';
import type {AuthScreensParamList, RootStackParamList} from '@libs/Navigation/types';
import toggleTestToolsModal from '@userActions/TestTool';
import CONST from '@src/CONST';
import CustomDevMenu from './CustomDevMenu';
Expand Down Expand Up @@ -94,7 +94,7 @@ type ScreenWrapperProps = {
*
* This is required because transitionEnd event doesn't trigger in the testing environment.
*/
navigation?: StackNavigationProp<RootStackParamList> | StackNavigationProp<CentralPaneNavigatorParamList>;
navigation?: StackNavigationProp<RootStackParamList> | StackNavigationProp<AuthScreensParamList>;
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?: StackNavigationProp<RootStackParamList> | StackNavigationProp<AuthScreensParamList>;
navigation?: StackNavigationProp<RootStackParamList> | StackNavigationProp<CentralPaneScreensParamList>;

Is it more correct if we use type CentralPaneScreensParamList here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CentralPaneScreensParamList is a subset of AuthScreens and it has been added to increase code readability and handle types correctly in the getCentralPaneScreenInitialParams function. For screens, I believe we should rely on the main set of screens.
cc: @adamgrzybowski

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the navigation object should be typed with all the screens from AuthNavigator | RootStack


/** Whether to show offline indicator on wide screens */
shouldShowOfflineIndicatorInWideScreen?: boolean;
Expand Down
4 changes: 2 additions & 2 deletions src/components/Search.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import * as ReportUtils from '@libs/ReportUtils';
import * as SearchUtils from '@libs/SearchUtils';
import type {SearchColumnType, SortOrder} from '@libs/SearchUtils';
import Navigation from '@navigation/Navigation';
import type {CentralPaneNavigatorParamList} from '@navigation/types';
import type {AuthScreensParamList} from '@navigation/types';
import EmptySearchView from '@pages/Search/EmptySearchView';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand Down Expand Up @@ -43,7 +43,7 @@ function isTransactionListItemType(item: TransactionListItemType | ReportListIte
function Search({query, policyIDs, sortBy, sortOrder}: SearchProps) {
const {isOffline} = useNetwork();
const styles = useThemeStyles();
const navigation = useNavigation<StackNavigationProp<CentralPaneNavigatorParamList>>();
const navigation = useNavigation<StackNavigationProp<AuthScreensParamList>>();
const lastSearchResultsRef = useRef<OnyxEntry<SearchResults>>();

const hash = SearchUtils.getQueryHash(query, policyIDs, sortBy, sortOrder);
Expand Down
27 changes: 27 additions & 0 deletions src/components/withPrepareCentralPaneScreen/index.native.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import type {ComponentType, ForwardedRef, RefAttributes} from 'react';
import React from 'react';
import getComponentDisplayName from '@libs/getComponentDisplayName';
import FreezeWrapper from '@libs/Navigation/FreezeWrapper';

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need some comments here and in the other file about what preparing means for native and web.

/**
* This HOC is dependent on platform. On native platforms screens that aren't already dipslayed in the navigation stack should be freezed to prevent from unnecessary rendering.
mountiny marked this conversation as resolved.
Show resolved Hide resolved
* It's handled this way only on mobile platforms, because on web more than one screen is displayed in a wide layout, so these screens shouldn't be freezed.
mountiny marked this conversation as resolved.
Show resolved Hide resolved
*/
export default function withPrepareCentralPaneScreen<TProps, TRef>(
WrappedComponent: ComponentType<TProps & RefAttributes<TRef>>,
): (props: TProps & React.RefAttributes<TRef>) => React.ReactElement | null {
function WithPrepareCentralPaneScreen(props: TProps, ref: ForwardedRef<TRef>) {
return (
<FreezeWrapper>
<WrappedComponent
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
ref={ref}
/>
</FreezeWrapper>
);
}

WithPrepareCentralPaneScreen.displayName = `WithPrepareCentralPaneScreen(${getComponentDisplayName(WrappedComponent)})`;
return React.forwardRef(WithPrepareCentralPaneScreen);
}
9 changes: 9 additions & 0 deletions src/components/withPrepareCentralPaneScreen/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type {ComponentType} from 'react';

/**
* This HOC is dependent on platform. On native platforms screens that aren't already dipslayed in the navigation stack should be freezed to prevent from unnecessary rendering.
mountiny marked this conversation as resolved.
Show resolved Hide resolved
* It's handled this way only on mobile platforms, because on web more than one screen is displayed in a wide layout, so these screens shouldn't be freezed.
mountiny marked this conversation as resolved.
Show resolved Hide resolved
*/
export default function withPrepareCentralPaneScreen(WrappedComponent: ComponentType) {
return WrappedComponent;
}
4 changes: 2 additions & 2 deletions src/hooks/useActiveRoute.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import {useContext} from 'react';
import ActiveRouteContext from '@libs/Navigation/AppNavigator/Navigators/ActiveRouteContext';
import type {CentralPaneNavigatorParamList, NavigationPartialRoute} from '@libs/Navigation/types';
import type {AuthScreensParamList, NavigationPartialRoute} from '@libs/Navigation/types';

function useActiveRoute(): NavigationPartialRoute<keyof CentralPaneNavigatorParamList> | undefined {
function useActiveRoute(): NavigationPartialRoute<keyof AuthScreensParamList> | undefined {
mountiny marked this conversation as resolved.
Show resolved Hide resolved
return useContext(ActiveRouteContext);
}

Expand Down
51 changes: 43 additions & 8 deletions src/libs/Navigation/AppNavigator/AuthScreens.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import React, {memo, useEffect, useMemo, useRef} from 'react';
import {View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import Onyx, {withOnyx} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import OptionsListContextProvider from '@components/OptionListContextProvider';
import withPrepareCentralPaneScreen from '@components/withPrepareCentralPaneScreen';
import useOnboardingLayout from '@hooks/useOnboardingLayout';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -14,7 +16,7 @@ import Log from '@libs/Log';
import getCurrentUrl from '@libs/Navigation/currentUrl';
import getOnboardingModalScreenOptions from '@libs/Navigation/getOnboardingModalScreenOptions';
import Navigation from '@libs/Navigation/Navigation';
import type {AuthScreensParamList} from '@libs/Navigation/types';
import type {AuthScreensParamList, CentralPaneName, CentralPaneScreensParamList} from '@libs/Navigation/types';
import NetworkConnection from '@libs/NetworkConnection';
import * as Pusher from '@libs/Pusher/pusher';
import PusherConnectionManager from '@libs/PusherConnectionManager';
Expand All @@ -40,11 +42,11 @@ import ROUTES from '@src/ROUTES';
import SCREENS from '@src/SCREENS';
import type * as OnyxTypes from '@src/types/onyx';
import type {SelectedTimezone, Timezone} from '@src/types/onyx/PersonalDetails';
import CENTRAL_PANE_SCREENS from './CENTRAL_PANE_SCREENS';
import createCustomStackNavigator from './createCustomStackNavigator';
import defaultScreenOptions from './defaultScreenOptions';
import getRootNavigatorScreenOptions from './getRootNavigatorScreenOptions';
import BottomTabNavigator from './Navigators/BottomTabNavigator';
import CentralPaneNavigator from './Navigators/CentralPaneNavigator';
import FeatureTrainingModalNavigator from './Navigators/FeatureTrainingModalNavigator';
import FullScreenNavigator from './Navigators/FullScreenNavigator';
import LeftModalNavigator from './Navigators/LeftModalNavigator';
Expand Down Expand Up @@ -73,6 +75,21 @@ const loadReportAvatar = () => require('../../../pages/ReportAvatar').default as
const loadReceiptView = () => require('../../../pages/TransactionReceiptPage').default as React.ComponentType;
const loadWorkspaceJoinUser = () => require('@pages/workspace/WorkspaceJoinUserPage').default as React.ComponentType;

function getCentralPaneScreenInitialParams(screenName: CentralPaneName): Partial<ValueOf<CentralPaneScreensParamList>> {
const url = getCurrentUrl();
const openOnAdminRoom = url ? new URL(url).searchParams.get('openOnAdminRoom') : undefined;

if (screenName === SCREENS.SEARCH.CENTRAL_PANE) {
return {sortBy: CONST.SEARCH_TABLE_COLUMNS.DATE, sortOrder: CONST.SORT_ORDER.DESC};
}

if (screenName === SCREENS.REPORT && openOnAdminRoom === 'true') {
return {openOnAdminRoom: true};
}

return undefined;
}

let timezone: Timezone | null;
let currentAccountID = -1;
let isLoadingApp = false;
Expand Down Expand Up @@ -284,20 +301,26 @@ function AuthScreens({session, lastOpenedPublicRoomID, initialLastUpdateIDApplie
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const CentralPaneScreenOptions = {
headerShown: false,
title: 'New Expensify',

// Prevent unnecessary scrolling
cardStyle: styles.cardStyleNavigator,
};

return (
<OptionsListContextProvider>
<View style={styles.rootNavigatorContainerStyles(isSmallScreenWidth)}>
<RootStack.Navigator isSmallScreenWidth={isSmallScreenWidth}>
<RootStack.Navigator
screenOptions={screenOptions.centralPaneNavigator}
isSmallScreenWidth={isSmallScreenWidth}
>
<RootStack.Screen
name={NAVIGATORS.BOTTOM_TAB_NAVIGATOR}
options={screenOptions.bottomTab}
component={BottomTabNavigator}
/>
<RootStack.Screen
name={NAVIGATORS.CENTRAL_PANE_NAVIGATOR}
options={screenOptions.centralPaneNavigator}
component={CentralPaneNavigator}
/>
<RootStack.Screen
name={SCREENS.VALIDATE_LOGIN}
options={{
Expand Down Expand Up @@ -419,6 +442,18 @@ function AuthScreens({session, lastOpenedPublicRoomID, initialLastUpdateIDApplie
options={defaultScreenOptions}
component={ConnectionCompletePage}
/>
{Object.entries(CENTRAL_PANE_SCREENS).map(([screenName, componentGetter]) => {
const centralPaneName = screenName as CentralPaneName;
return (
<RootStack.Screen
key={centralPaneName}
name={centralPaneName}
initialParams={getCentralPaneScreenInitialParams(centralPaneName)}
getComponent={() => withPrepareCentralPaneScreen(componentGetter())}
options={CentralPaneScreenOptions}
/>
);
})}
</RootStack.Navigator>
</View>
</OptionsListContextProvider>
Expand Down
20 changes: 20 additions & 0 deletions src/libs/Navigation/AppNavigator/CENTRAL_PANE_SCREENS.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import type {CentralPaneName} from '@libs/Navigation/types';
import SCREENS from '@src/SCREENS';

type Screens = Partial<Record<CentralPaneName, () => React.ComponentType>>;

const CENTRAL_PANE_SCREENS = {
Copy link
Contributor

@ZhenjaHorbach ZhenjaHorbach Jul 10, 2024

Choose a reason for hiding this comment

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

BugZero Checklist

These changes caused this issue
This issue was caused by invoking withPrepareCentralPaneScreen HOC multiple times in CENTRAL_PANE_SCREENS.tsx

[SCREENS.SETTINGS.WORKSPACES]: () => require('../../../pages/workspace/WorkspacesListPage').default as React.ComponentType,
[SCREENS.SETTINGS.PREFERENCES.ROOT]: () => require('../../../pages/settings/Preferences/PreferencesPage').default as React.ComponentType,
[SCREENS.SETTINGS.SECURITY]: () => require('../../../pages/settings/Security/SecuritySettingsPage').default as React.ComponentType,
[SCREENS.SETTINGS.PROFILE.ROOT]: () => require('../../../pages/settings/Profile/ProfilePage').default as React.ComponentType,
[SCREENS.SETTINGS.WALLET.ROOT]: () => require('../../../pages/settings/Wallet/WalletPage').default as React.ComponentType,
[SCREENS.SETTINGS.ABOUT]: () => require('../../../pages/settings/AboutPage/AboutPage').default as React.ComponentType,
[SCREENS.SETTINGS.TROUBLESHOOT]: () => require('../../../pages/settings/Troubleshoot/TroubleshootPage').default as React.ComponentType,
[SCREENS.SETTINGS.SAVE_THE_WORLD]: () => require('../../../pages/TeachersUnite/SaveTheWorldPage').default as React.ComponentType,
[SCREENS.SETTINGS.SUBSCRIPTION.ROOT]: () => require('../../../pages/settings/Subscription/SubscriptionSettingsPage').default as React.ComponentType,
[SCREENS.SEARCH.CENTRAL_PANE]: () => require('../../../pages/Search/SearchPage').default as React.ComponentType,
[SCREENS.REPORT]: () => require('./ReportScreenWrapper').default as React.ComponentType,
} satisfies Screens;

export default CENTRAL_PANE_SCREENS;
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import type {CentralPaneNavigatorParamList, NavigationPartialRoute} from '@libs/Navigation/types';
import type {AuthScreensParamList, NavigationPartialRoute} from '@libs/Navigation/types';

const ActiveRouteContext = React.createContext<NavigationPartialRoute<keyof CentralPaneNavigatorParamList> | undefined>(undefined);
const ActiveRouteContext = React.createContext<NavigationPartialRoute<keyof AuthScreensParamList> | undefined>(undefined);

export default ActiveRouteContext;

This file was deleted.

This file was deleted.

This file was deleted.

4 changes: 2 additions & 2 deletions src/libs/Navigation/AppNavigator/ReportScreenWrapper.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import type {StackScreenProps} from '@react-navigation/stack';
import React from 'react';
import type {CentralPaneNavigatorParamList} from '@navigation/types';
import type {AuthScreensParamList} from '@navigation/types';
import ReportScreen from '@pages/home/ReportScreen';
import type SCREENS from '@src/SCREENS';
import ReportScreenIDSetter from './ReportScreenIDSetter';

type ReportScreenWrapperProps = StackScreenProps<CentralPaneNavigatorParamList, typeof SCREENS.REPORT>;
type ReportScreenWrapperProps = StackScreenProps<AuthScreensParamList, typeof SCREENS.REPORT>;
mountiny marked this conversation as resolved.
Show resolved Hide resolved

function ReportScreenWrapper({route, navigation}: ReportScreenWrapperProps) {
// The ReportScreen without the reportID set will display a skeleton
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import getTopmostBottomTabRoute from '@libs/Navigation/getTopmostBottomTabRoute'
import getTopmostCentralPaneRoute from '@libs/Navigation/getTopmostCentralPaneRoute';
import Navigation from '@libs/Navigation/Navigation';
import type {RootStackParamList, State} from '@libs/Navigation/types';
import isCentralPaneName from '@libs/NavigationUtils';
import {getChatTabBrickRoad} from '@libs/WorkspacesSettingsUtils';
import BottomTabAvatar from '@pages/home/sidebar/BottomTabAvatar';
import BottomTabBarFloatingActionButton from '@pages/home/sidebar/BottomTabBarFloatingActionButton';
Expand Down Expand Up @@ -46,7 +47,7 @@ function BottomTabBar({isLoadingApp = false}: PurposeForUsingExpensifyModalProps
const currentRoute = routes?.[navigationState?.index ?? 0];
// When we are redirected to the Settings tab from the OldDot, we don't want to call the Welcome.show() method.
// To prevent this, the value of the bottomTabRoute?.name is checked here
if (!!(currentRoute && currentRoute.name !== NAVIGATORS.BOTTOM_TAB_NAVIGATOR && currentRoute.name !== NAVIGATORS.CENTRAL_PANE_NAVIGATOR) || Session.isAnonymousUser()) {
if (!!(currentRoute && currentRoute.name !== NAVIGATORS.BOTTOM_TAB_NAVIGATOR && !isCentralPaneName(currentRoute.name)) || Session.isAnonymousUser()) {
return;
}

Expand Down
Loading
Loading