Skip to content

[Wave 8] [Ideal Nav] Fix iPad navigation #35845

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
mountiny merged 2 commits into from
Feb 8, 2024
Merged

[Wave 8] [Ideal Nav] Fix iPad navigation #35845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b230459
add isNarrowLayout
adamgrzybowski Feb 5, 2024
de6bc5e
add isNarrowLayout to switchPolicyID
adamgrzybowski Feb 8, 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
6 changes: 3 additions & 3 deletions src/libs/Navigation/AppNavigator/createCustomFullScreenNavigator/CustomFullScreenRouter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type {ParamListBase, PartialState, RouterConfigOptions, StackNavigationState} from '@react-navigation/native';
import {StackRouter} from '@react-navigation/native';
import getIsSmallScreenWidth from '@libs/getIsSmallScreenWidth';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import SCREENS from '@src/SCREENS';
import type {FullScreenNavigatorRouterOptions} from './types';

Expand All @@ -9,11 +9,11 @@ type StackState = StackNavigationState<ParamListBase> | PartialState<StackNaviga
const isAtLeastOneInState = (state: StackState, screenName: string): boolean => !!state.routes.find((route) => route.name === screenName);

function adaptStateIfNecessary(state: StackState) {
const isSmallScreenWidth = getIsSmallScreenWidth();
const isNarrowLayout = getIsNarrowLayout();
// If the screen is wide, there should be at least two screens inside:
// - SETINGS.ROOT to cover left pane.
// - SETTINGS_CENTRAL_PANE to cover central pane.
if (!isSmallScreenWidth) {
if (!isNarrowLayout) {
if (!isAtLeastOneInState(state, SCREENS.SETTINGS.ROOT)) {
// @ts-expect-error Updating read only property
// noinspection JSConstantReassignment
Expand Down
6 changes: 3 additions & 3 deletions src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {RouterConfigOptions, StackNavigationState} from '@react-navigation/native';
import {getPathFromState, StackRouter} from '@react-navigation/native';
import type {ParamListBase} from '@react-navigation/routers';
import getIsSmallScreenWidth from '@libs/getIsSmallScreenWidth';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import getTopmostBottomTabRoute from '@libs/Navigation/getTopmostBottomTabRoute';
import getTopmostCentralPaneRoute from '@libs/Navigation/getTopmostCentralPaneRoute';
import linkingConfig from '@libs/Navigation/linkingConfig';
Expand Down Expand Up @@ -38,10 +38,10 @@ function compareAndAdaptState(state: StackNavigationState<RootStackParamList>) {

// We need to be sure that the bottom tab state is defined.
const topmostBottomTabRoute = getTopmostBottomTabRoute(state);
const isSmallScreenWidth = getIsSmallScreenWidth();
const isNarrowLayout = getIsNarrowLayout();

// This solutions is heuristics and will work for our cases. We may need to improve it in the future if we will have more cases to handle.
if (topmostBottomTabRoute && !isSmallScreenWidth) {
if (topmostBottomTabRoute && !isNarrowLayout) {
const fullScreenRoute = state.routes.find((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR);

// If there is fullScreenRoute we don't need to add anything.
Expand Down
4 changes: 2 additions & 2 deletions src/libs/Navigation/linkTo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {getActionFromState} from '@react-navigation/core';
import type {NavigationAction, NavigationContainerRef, NavigationState, PartialState} from '@react-navigation/native';
import type {Writable} from 'type-fest';
import getIsSmallScreenWidth from '@libs/getIsSmallScreenWidth';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import {extractPolicyIDFromPath, getPathWithoutPolicyID} from '@libs/PolicyUtils';
import CONST from '@src/CONST';
import NAVIGATORS from '@src/NAVIGATORS';
Expand Down Expand Up @@ -212,7 +212,7 @@ export default function linkTo(navigation: NavigationContainerRef<RootStackParam
root.dispatch(actionForBottomTabNavigator);

// If the layout is wide we need to push matching central pane route to the stack.
if (!getIsSmallScreenWidth()) {
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)!;
Expand Down
10 changes: 5 additions & 5 deletions src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {NavigationState, PartialState} from '@react-navigation/native';
import {getStateFromPath} from '@react-navigation/native';
import {isAnonymousUser} from '@libs/actions/Session';
import getIsSmallScreenWidth from '@libs/getIsSmallScreenWidth';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import getTopmostNestedRHPRoute from '@libs/Navigation/getTopmostNestedRHPRoute';
import type {BottomTabName, CentralPaneName, FullScreenName, NavigationPartialRoute, RootStackParamList} from '@libs/Navigation/types';
import {extractPolicyIDFromPath, getPathWithoutPolicyID} from '@libs/PolicyUtils';
Expand Down Expand Up @@ -132,7 +132,7 @@ function getMatchingRootRouteForRHPRoute(
}

function getAdaptedState(state: PartialState<NavigationState<RootStackParamList>>, policyID?: string): GetAdaptedStateReturnType {
const isSmallScreenWidth = getIsSmallScreenWidth();
const isNarrowLayout = getIsNarrowLayout();
const metainfo = {
isCentralPaneAndBottomTabMandatory: true,
isFullScreenNavigatorMandatory: true,
Expand Down Expand Up @@ -194,7 +194,7 @@ function getAdaptedState(state: PartialState<NavigationState<RootStackParamList>
policyID,
),
);
if (!isSmallScreenWidth) {
if (!isNarrowLayout) {
routes.push(
createCentralPaneNavigator({
name: SCREENS.REPORT,
Expand Down Expand Up @@ -226,7 +226,7 @@ function getAdaptedState(state: PartialState<NavigationState<RootStackParamList>
policyID,
),
);
if (!isSmallScreenWidth) {
if (!isNarrowLayout) {
routes.push(createCentralPaneNavigator({name: SCREENS.REPORT}));
}
routes.push(fullScreenNavigator);
Expand Down Expand Up @@ -254,7 +254,7 @@ function getAdaptedState(state: PartialState<NavigationState<RootStackParamList>
// Routes
// - found bottom tab
// - matching central pane on desktop layout
if (isSmallScreenWidth) {
if (isNarrowLayout) {
return {
adaptedState: state,
metainfo,
Expand Down
4 changes: 2 additions & 2 deletions src/libs/Navigation/switchPolicyID.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {getActionFromState} from '@react-navigation/core';
import type {NavigationAction, NavigationContainerRef, NavigationState, PartialState} from '@react-navigation/native';
import {getPathFromState} from '@react-navigation/native';
import type {ValueOf, Writable} from 'type-fest';
import getIsSmallScreenWidth from '@libs/getIsSmallScreenWidth';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import CONST from '@src/CONST';
import NAVIGATORS from '@src/NAVIGATORS';
import type {Route} from '@src/ROUTES';
Expand Down Expand Up @@ -95,7 +95,7 @@ export default function switchPolicyID(navigation: NavigationContainerRef<RootSt
}

// If the layout is wide we need to push matching central pane route to the stack.
if (!getIsSmallScreenWidth()) {
if (!getIsNarrowLayout()) {
// Case when the user selects "All" workspace from the specific workspace settings
if (checkIfActionPayloadNameIsEqual(actionForBottomTabNavigator, SCREENS.ALL_SETTINGS) && !policyID) {
root.dispatch({
Expand Down
3 changes: 3 additions & 0 deletions src/libs/getIsNarrowLayout/index.native.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function getIsNarrowLayout() {
return true;

@roryabraham roryabraham Feb 5, 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 is an unsafe assumption – there are devices (such as an iPad pro), where even in portrait layout the screen width is wider than the breakpoint for narrow screens.

I think this practice of using platform as a proxy for other device properties which can be measured directly is bad and a source for technical debt.

I feel strongly that this PR be closed and we fix this by addressing the root cause of the problem(s) on iPad. My hunch is that most of them are stemming from bad assumptions like this one that native code must be running on a device with narrow screen width.

@mountiny mountiny Feb 5, 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.

I feel like this is more of a feature request as the assumption of us forcing portrait mode and narrow layout is running for a while and until we decide to officially support landscape too, its fine to keep it in the app.

@mountiny mountiny Feb 5, 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.

bad assumptions like this one that native code must be running on a device with narrow screen width.

This is not bad assumption imho, this is a intentional product/ design choice made in the past

@situchan situchan Feb 5, 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.

even if the screen width is beyond the threshold, the iPad always displays narrow layout

We already applied this throughout the app so I think it's fine for now.
Agree we should completely refactor this bad logic when we support wide layout in portrait mode of super large iPad (maybe when we officially support landscape mode)

}
5 changes: 5 additions & 0 deletions src/libs/getIsNarrowLayout/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import getIsSmallScreenWidth from '@libs/getIsSmallScreenWidth';

export default function getIsNarrowLayout() {
return getIsSmallScreenWidth();
}