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

Improve goBack function #26498

Merged
merged 16 commits into from
Nov 13, 2023
Merged

Improve goBack function #26498

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f747ae9
improve goBack function
adamgrzybowski Sep 1, 2023
0644669
Merge branch 'main' into @swm/improve-goBack-with-fallback-route
adamgrzybowski Sep 25, 2023
881dc84
fix typo
adamgrzybowski Sep 25, 2023
3fb6f8e
Merge branch 'main' into @swm/improve-goBack-with-fallback-route
adamgrzybowski Sep 25, 2023
59c3cdc
fix tests
adamgrzybowski Sep 25, 2023
d672079
Merge branch 'main' into @swm/improve-goBack-with-fallback-route
adamgrzybowski Oct 26, 2023
5fd8ec9
limit search for path to 5 screens from the top
adamgrzybowski Oct 26, 2023
6d90b44
Merge branch 'main' into @swm/improve-goBack-with-fallback-route
WojtekBoman Nov 6, 2023
fac03c4
Add OptionRowLHNDataReport
WojtekBoman Nov 7, 2023
201ea6d
Merge branch 'main' into @swm/improve-goBack-with-fallback-route
WojtekBoman Nov 7, 2023
22114be
Fix propTypes in OptionRowLHNData
WojtekBoman Nov 7, 2023
35b4e44
Fix flatList position after deleting money requests
WojtekBoman Nov 8, 2023
e1ca40f
Merge branch 'main' into @swm/improve-goBack-with-fallback-route
WojtekBoman Nov 9, 2023
f1ba1c7
Merge branch 'main' into @swm/improve-goBack-with-fallback-route
WojtekBoman Nov 10, 2023
aec50d8
Remove the useFocusEffect hook from ReportActionsList
WojtekBoman Nov 10, 2023
057f055
Add AUTOSCROLL_TO_TOP_THRESHOLD const
WojtekBoman Nov 13, 2023
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
37 changes: 35 additions & 2 deletions src/libs/Navigation/Navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import _ from 'lodash';
import lodashGet from 'lodash/get';
import {CommonActions, getPathFromState, StackActions} from '@react-navigation/native';
import {getActionFromState} from '@react-navigation/core';
import {getActionFromState, findFocusedRoute} from '@react-navigation/core';
import Log from '../Log';
import DomUtils from '../DomUtils';
import linkTo from './linkTo';
Expand Down Expand Up @@ -70,6 +70,24 @@ const getActiveRouteIndex = function (route, index) {
return index;
};

function getPathsForRoutesInRootNavigator() {
let currentState = navigationRef.getRootState();
const paths = [];

while (currentState.routes.length > 0) {
try {
const path = getPathFromState(currentState, linkingConfig.config);
if (path) {
paths.push(path.substring(1));
}
} catch (e) {
return paths;
}
currentState = {...currentState, routes: currentState.routes.slice(0, -1), index: currentState.index - 1};
}
return paths;
}
Copy link
Contributor

@s77rt s77rt Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this may cause a performance degradation for users who keep using the app for long periods. The more you use the app the more paths we have to extract. Most of the paths we get will be a waste. I suggest to implement an early return mechanism.

function getIndexOfPathInRootNavigator(path) {
    let currentState = navigationRef.getRootState();

    for (let index = 0; index < 5; index++) {
        if (!currentState.routes.length) {
            break;
        }

        const pathFromState = getPathFromState(currentState, linkingConfig.config);
        if (path === pathFromState.substring(1)) {
            return index;
        }

        currentState = {...currentState, routes: currentState.routes.slice(0, -1), index: currentState.index - 1};
    }

    return -1;
}

The 5 limit is used as a threshold. There would be no case where we would pop more than 4 screens. Currently I think the max to pop is 2. So maybe drop the 5 to 3.


/**
* Main navigation method for redirecting to a route.
* @param {String} route
Expand Down Expand Up @@ -116,7 +134,6 @@ function goBack(fallbackRoute, shouldEnforceFallback = false, shouldPopToTop = f
}

const isFirstRouteInNavigator = !getActiveRouteIndex(navigationRef.current.getState());

if (isFirstRouteInNavigator) {
const rootState = navigationRef.getRootState();
const lastRoute = _.last(rootState.routes);
Expand All @@ -132,6 +149,22 @@ function goBack(fallbackRoute, shouldEnforceFallback = false, shouldPopToTop = f
return;
}

const isCentralPaneFocused = findFocusedRoute(navigationRef.current.getState()).name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR;
const pathsForRoutesInRootNavigator = getPathsForRoutesInRootNavigator();

// Allow CentralPane to use goBack with fallback route.
if (isCentralPaneFocused && fallbackRoute && !pathsForRoutesInRootNavigator.includes(fallbackRoute)) {
navigate(fallbackRoute, CONST.NAVIGATION.TYPE.FORCED_UP);
return;
}

// Add posibility to go back more than one screen in root navigator.
if (isCentralPaneFocused && fallbackRoute && pathsForRoutesInRootNavigator.indexOf(fallbackRoute) > 0) {
const popNumber = pathsForRoutesInRootNavigator.indexOf(fallbackRoute);
navigationRef.current.dispatch(StackActions.pop(popNumber));
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB. Define the indexOfFallbackRoute once.


navigationRef.current.goBack();
}

Expand Down
6 changes: 2 additions & 4 deletions src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1482,15 +1482,13 @@ function deleteMoneyRequest(transactionID, reportAction, isSingleTransactionView
// STEP 7: Navigate the user depending on which page they are on and which resources were deleted
if (isSingleTransactionView && shouldDeleteTransactionThread && !shouldDeleteIOUReport) {
// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
Navigation.goBack(ROUTES.HOME);
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(iouReport.reportID));
Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(iouReport.reportID));
return;
}

if (shouldDeleteIOUReport) {
// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
Navigation.goBack(ROUTES.HOME);
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(iouReport.chatReportID));
Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(iouReport.chatReportID));
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/actions/IOUTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2148,14 +2148,14 @@ describe('actions/IOU', () => {

// Then we expect to navigate to the iou report

expect(Navigation.navigate).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(IOU_REPORT_ID));
expect(Navigation.goBack).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(IOU_REPORT_ID));
});

it('navigate the user correctly to the chat Report when appropriate', () => {
// When we delete the money request and we should delete the IOU report
IOU.deleteMoneyRequest(transaction.transactionID, createIOUAction, false);
// Then we expect to navigate to the chat report
expect(Navigation.navigate).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(chatReport.reportID));
expect(Navigation.goBack).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(chatReport.reportID));
});
});
});