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

Add helper functions in getMatchingCentralPaneRouteForState #36397

Merged
merged 4 commits into from
Feb 22, 2024
Merged

Add helper functions in getMatchingCentralPaneRouteForState #36397

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
833ea1f
Add helper functions in getMatchingCentralPaneRouteForState
WojtekBoman Feb 13, 2024
7d18914
Refactor getMatchingCentralPaneRouteForState
WojtekBoman Feb 13, 2024
49049a5
Merge branch 'main' into ideal-nav/go-back-to-settings-page
WojtekBoman Feb 21, 2024
b21f8d7
Refactor hasRouteMatchingPolicyID and getAlreadyOpenedSettingsScreen
WojtekBoman Feb 21, 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/libs/Navigation/linkTo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ export default function linkTo(navigation: NavigationContainerRef<RootStackParam
if (!getIsNarrowLayout()) {
// stateFromPath should always include bottom tab navigator state, so getMatchingCentralPaneRouteForState will be always defined.
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const matchingCentralPaneRoute = getMatchingCentralPaneRouteForState(stateFromPath)!;
const matchingCentralPaneRoute = getMatchingCentralPaneRouteForState(stateFromPath, rootState)!;
if (matchingCentralPaneRoute && 'name' in matchingCentralPaneRoute) {
root.dispatch({
type: CONST.NAVIGATION.ACTION_TYPE.PUSH,
Expand Down
46 changes: 43 additions & 3 deletions src/libs/Navigation/linkingConfig/getMatchingCentralPaneRouteForState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import getTopmostBottomTabRoute from '@libs/Navigation/getTopmostBottomTabRoute';
import type {CentralPaneName, NavigationPartialRoute, RootStackParamList, State} from '@libs/Navigation/types';
import type {CentralPaneName, CentralPaneNavigatorParamList, NavigationPartialRoute, RootStackParamList, State} from '@libs/Navigation/types';
import NAVIGATORS from '@src/NAVIGATORS';
import SCREENS from '@src/SCREENS';
import TAB_TO_CENTRAL_PANE_MAPPING from './TAB_TO_CENTRAL_PANE_MAPPING';
Expand Down Expand Up @@ -31,8 +31,45 @@ const getTopMostReportIDFromRHP = (state: State): string => {
return '';
};

// Function that checks if the given route has a policyID equal to the id provided in the function params
Copy link
Contributor

Choose a reason for hiding this comment

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

When commenting functions, we don't focus on the fact that they're functions. We either write "Do something such and such" or "Does something and something".

function hasRouteMatchingPolicyID(route: NavigationPartialRoute<CentralPaneName>, policyID?: string) {
return (
route?.params &&
'params' in route?.params &&
route.params.params &&
'policyID' in (route.params.params as Record<string, string | undefined>) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't type assert this twice, extract a local variable of desired target type.

(route.params.params as Record<string, string | undefined>).policyID === policyID
);
}

// Function that restores already opened screen in the settings tab. Thanks to this function, we are able to open the already selected screen in the workspace settings when we return from another tab.
function getAlreadyOpenedSettingsScreen(rootState?: State, policyID?: string): keyof CentralPaneNavigatorParamList | undefined {
if (!rootState) {
return undefined;
}

// If one of the screen from WORKSPACES_SCREENS is now in the navigation state, we can decide which screen we should display.
// A screen from the navigation state can be pushed to the navigation state again only if it has a matching policyID with the currently selected workspace.
// Otherwise, when we switch the workspace, we want to display the initial screen in the settings tab.
const WORKSPACES_SCREENS = TAB_TO_CENTRAL_PANE_MAPPING[SCREENS.WORKSPACE.INITIAL].concat(TAB_TO_CENTRAL_PANE_MAPPING[SCREENS.ALL_SETTINGS]);
Copy link
Contributor

Choose a reason for hiding this comment

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

All local variables in the project are named using lower camel case convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

But why is this local, if this doesn't depend on function arguments?


const alreadyOpenedSettingsTab = rootState.routes
.filter((item) => item.params && 'screen' in item.params && WORKSPACES_SCREENS.includes(item.params.screen as keyof CentralPaneNavigatorParamList))
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of type assertion going on (here and in other places). I gave it a quick look and I couldn't come up with any quick fixes. Do you have any ideas how we could at least reduce the number of type assertions (something as SomeType)?

Copy link
Contributor Author

@WojtekBoman WojtekBoman Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I am trying to figure it out, but it's not easy because the screen is unknown and when we want to use this variable we need to have a type for it. I'll try to solve it somehow, but I don't know if we can avoid using type assertions here. Could you take a look at the latest update in this PR? It should resolve the comments above :)

.at(-1);

if (
alreadyOpenedSettingsTab?.params &&
'screen' in alreadyOpenedSettingsTab?.params &&
hasRouteMatchingPolicyID(alreadyOpenedSettingsTab as NavigationPartialRoute<CentralPaneName>, policyID)
) {
return alreadyOpenedSettingsTab?.params?.screen as keyof CentralPaneNavigatorParamList;
}

return undefined;
}

// Get matching central pane route for bottom tab navigator. e.g HOME -> REPORT
function getMatchingCentralPaneRouteForState(state: State<RootStackParamList>): NavigationPartialRoute<CentralPaneName> | undefined {
function getMatchingCentralPaneRouteForState(state: State<RootStackParamList>, rootState?: State): NavigationPartialRoute<CentralPaneName> | undefined {
const topmostBottomTabRoute = getTopmostBottomTabRoute(state);

if (!topmostBottomTabRoute) {
Expand All @@ -42,7 +79,10 @@ function getMatchingCentralPaneRouteForState(state: State<RootStackParamList>):
const centralPaneName = TAB_TO_CENTRAL_PANE_MAPPING[topmostBottomTabRoute.name][0];

if (topmostBottomTabRoute.name === SCREENS.WORKSPACE.INITIAL) {
return {name: centralPaneName, params: topmostBottomTabRoute.params};
// When we go back to the settings tab without switching the workspace id, we want to return to the previously opened screen
const policyID = topmostBottomTabRoute?.params && 'policyID' in topmostBottomTabRoute?.params ? (topmostBottomTabRoute.params.policyID as string) : undefined;
const screen = getAlreadyOpenedSettingsScreen(rootState, policyID) ?? centralPaneName;
return {name: screen, params: topmostBottomTabRoute.params};
}

if (topmostBottomTabRoute.name === SCREENS.HOME) {
Expand Down
Loading