From d1a2aa5c825d14ba9c8b43ecf69fca9f13dba393 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 12 Apr 2021 15:45:00 +0300 Subject: [PATCH 1/8] refactor: NavigationRoot extract `renderFallback` --- src/libs/Navigation/NavigationRoot.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/libs/Navigation/NavigationRoot.js b/src/libs/Navigation/NavigationRoot.js index a7e781abecb4..af069c741e4e 100644 --- a/src/libs/Navigation/NavigationRoot.js +++ b/src/libs/Navigation/NavigationRoot.js @@ -1,6 +1,6 @@ import _ from 'underscore'; import React, {Component} from 'react'; -import {ActivityIndicator, Linking, View} from 'react-native'; +import {Linking} from 'react-native'; import PropTypes from 'prop-types'; import { getStateFromPath, @@ -14,10 +14,9 @@ import AppNavigator from './AppNavigator'; import getPathName from './getPathName'; import ONYXKEYS from '../../ONYXKEYS'; import ROUTES from '../../ROUTES'; -import styles from '../../styles/styles'; -import themeColors from '../../styles/themes/default'; import {updateCurrentlyViewedReportID} from '../actions/Report'; import {setCurrentURL} from '../actions/App'; +import FullScreenLoadingIndicator from '../../components/FullscreenLoadingIndicator'; const propTypes = { // Whether the current user is logged in with an authToken @@ -82,13 +81,17 @@ class NavigationRoot extends Component { }); } + /** + * Render some fallback content until the navigation is ready + * @returns {JSX.Element} + */ + renderFallback() { + return ; + } + render() { if (this.state.loading) { - return ( - - - - ); + return this.renderFallback(); } // If we are on web, desktop, or a widescreen width we will use our custom navigator to create the wider layout From ceac113bfe45d96cad4101b2783f32d8e9c3e4d1 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 12 Apr 2021 16:06:12 +0300 Subject: [PATCH 2/8] refactor: NavigationRoot extract change handler --- src/libs/Navigation/NavigationRoot.js | 38 ++++++++++++++++----------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/libs/Navigation/NavigationRoot.js b/src/libs/Navigation/NavigationRoot.js index af069c741e4e..5d58eeb9bd90 100644 --- a/src/libs/Navigation/NavigationRoot.js +++ b/src/libs/Navigation/NavigationRoot.js @@ -38,6 +38,8 @@ class NavigationRoot extends Component { loading: true, initialState: undefined, }; + + this.handleStateChange = this.handleStateChange.bind(this); } componentDidMount() { @@ -81,6 +83,26 @@ class NavigationRoot extends Component { }); } + /** + * Intercept state changes and perform different logic + * @param {NavigationState} state + */ + handleStateChange(state) { + if (!state) { + return; + } + + const path = getPathFromState(state, linkingConfig.config); + if (path.includes(ROUTES.REPORT)) { + const reportID = Number(_.last(path.split('/'))); + if (reportID && !_.isNaN(reportID)) { + updateCurrentlyViewedReportID(reportID); + } + } + + setCurrentURL(path); + } + /** * Render some fallback content until the navigation is ready * @returns {JSX.Element} @@ -98,21 +120,7 @@ class NavigationRoot extends Component { return ( { - if (!state) { - return; - } - - const path = getPathFromState(state, linkingConfig.config); - if (path.includes(ROUTES.REPORT)) { - const reportID = Number(_.last(path.split('/'))); - if (reportID && !_.isNaN(reportID)) { - updateCurrentlyViewedReportID(reportID); - } - } - - setCurrentURL(path); - }} + onStateChange={this.handleStateChange} ref={navigationRef} linking={linkingConfig} documentTitle={{ From 0c539481c44fe57ab0fca0878a81c19c7c780b2d Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 12 Apr 2021 17:17:47 +0300 Subject: [PATCH 3/8] refactor: NavigationRoot remove Linking handling With the current configuration it should be possible for the navigation to resolve initial state entirely through `linkingConfig` --- src/libs/Navigation/NavigationRoot.js | 85 +-------------------------- 1 file changed, 3 insertions(+), 82 deletions(-) diff --git a/src/libs/Navigation/NavigationRoot.js b/src/libs/Navigation/NavigationRoot.js index 5d58eeb9bd90..0b6707b70a0f 100644 --- a/src/libs/Navigation/NavigationRoot.js +++ b/src/libs/Navigation/NavigationRoot.js @@ -1,18 +1,10 @@ import _ from 'underscore'; import React, {Component} from 'react'; -import {Linking} from 'react-native'; import PropTypes from 'prop-types'; -import { - getStateFromPath, - getPathFromState, - NavigationContainer, -} from '@react-navigation/native'; -import {withOnyx} from 'react-native-onyx'; +import {getPathFromState, NavigationContainer} from '@react-navigation/native'; import {navigationRef} from './Navigation'; import linkingConfig from './linkingConfig'; import AppNavigator from './AppNavigator'; -import getPathName from './getPathName'; -import ONYXKEYS from '../../ONYXKEYS'; import ROUTES from '../../ROUTES'; import {updateCurrentlyViewedReportID} from '../actions/Report'; import {setCurrentURL} from '../actions/App'; @@ -21,68 +13,15 @@ import FullScreenLoadingIndicator from '../../components/FullscreenLoadingIndica const propTypes = { // Whether the current user is logged in with an authToken authenticated: PropTypes.bool.isRequired, - - // The current reportID that we are navigated to or should show in the ReportScreen - currentlyViewedReportID: PropTypes.string, -}; - -const defaultProps = { - currentlyViewedReportID: null, }; class NavigationRoot extends Component { constructor(props) { super(props); - this.state = { - loading: true, - initialState: undefined, - }; - this.handleStateChange = this.handleStateChange.bind(this); } - componentDidMount() { - Linking.getInitialURL() - .then((initialUrl) => { - // On web we should be able to parse this. It will be null on native for now until deep links are - // hooked up - const path = getPathName(initialUrl); - let initialState = getStateFromPath(path, linkingConfig.config); - setCurrentURL(path); - - // If we are landing on something other than the report screen or site root then we MUST set the - // initial route to the currently viewed report so there some history to navigate back from - if (path !== `/${ROUTES.HOME}` && !path.includes(`/${ROUTES.REPORT}`)) { - const homeRoute = { - name: 'Home', - }; - - if (this.props.currentlyViewedReportID) { - homeRoute.params = { - screen: 'Report', - params: { - reportID: this.props.currentlyViewedReportID, - }, - }; - } - - if (!initialState) { - initialState = { - routes: [], - }; - } - - initialState.routes = [ - homeRoute, - ...initialState.routes, - ]; - } - - this.setState({loading: false, initialState}); - }); - } - /** * Intercept state changes and perform different logic * @param {NavigationState} state @@ -103,23 +42,10 @@ class NavigationRoot extends Component { setCurrentURL(path); } - /** - * Render some fallback content until the navigation is ready - * @returns {JSX.Element} - */ - renderFallback() { - return ; - } - render() { - if (this.state.loading) { - return this.renderFallback(); - } - - // If we are on web, desktop, or a widescreen width we will use our custom navigator to create the wider layout return ( } onStateChange={this.handleStateChange} ref={navigationRef} linking={linkingConfig} @@ -134,9 +60,4 @@ class NavigationRoot extends Component { } NavigationRoot.propTypes = propTypes; -NavigationRoot.defaultProps = defaultProps; -export default withOnyx({ - currentlyViewedReportID: { - key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID, - }, -})(NavigationRoot); +export default NavigationRoot; From d47683ea2258326f571a5346fe5830b6dea10dab Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 12 Apr 2021 17:26:25 +0300 Subject: [PATCH 4/8] feat: AuthScreens setup initial Home screen params 1. Either initial report data is available right from the start from Onyx 2. Or it is fetched and restored by `fetchAllReports` 3. Or if all else fails start with no report opened --- .../Navigation/AppNavigator/AuthScreens.js | 41 ++++++++++++++++++- src/libs/Navigation/linkingConfig.js | 2 +- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js index 3f4f2c5ee360..9f13d9261105 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.js +++ b/src/libs/Navigation/AppNavigator/AuthScreens.js @@ -56,11 +56,16 @@ const modalScreenListeners = { const propTypes = { network: PropTypes.shape({isOffline: PropTypes.bool}), + + /* The initial report for the home screen */ + initialReportID: PropTypes.string, + ...windowDimensionsPropTypes, }; const defaultProps = { network: {isOffline: true}, + initialReportID: null, }; class AuthScreens extends React.Component { @@ -69,6 +74,10 @@ class AuthScreens extends React.Component { Timing.start(CONST.TIMING.HOMEPAGE_INITIAL_RENDER); Timing.start(CONST.TIMING.HOMEPAGE_REPORTS_LOADED); + + if (props.initialReportID) { + this.setInitialHomeParams(props.initialReportID); + } } componentDidMount() { @@ -109,11 +118,20 @@ class AuthScreens extends React.Component { }, ['meta'], true); } - shouldComponentUpdate(prevProps) { - if (prevProps.isSmallScreenWidth !== this.props.isSmallScreenWidth) { + shouldComponentUpdate(nextProps) { + if (nextProps.isSmallScreenWidth !== this.props.isSmallScreenWidth) { return true; } + // Update initialHomeParams only once + if (!this.initialHomeParams) { + // Either we have a reportID or fetchAllReports resolved with no reports. Otherwise keep waiting + if (nextProps.initialReportID || nextProps.initialReportID === '') { + this.setInitialHomeParams(nextProps.initialReportID); + return true; + } + } + return false; } @@ -124,7 +142,22 @@ class AuthScreens extends React.Component { this.interval = null; } + /** + * Setting the initial params would update the URL in the address bar to correctly + * It would also setup correct initial state for the Home screen + * + * @param {String} reportID + */ + setInitialHomeParams(reportID) { + this.initialHomeParams = { + screen: 'Report', + params: {reportID}, + }; + } + render() { + if (!this.initialHomeParams) { return null; } + const modalScreenOptions = { headerShown: false, cardStyle: getNavigationModalCardStyle(this.props.isSmallScreenWidth), @@ -147,6 +180,7 @@ class AuthScreens extends React.Component { headerShown: false, title: 'Expensify.cash', }} + initialParams={this.initialHomeParams} component={MainDrawerNavigator} /> @@ -210,5 +244,8 @@ export default compose( network: { key: ONYXKEYS.NETWORK, }, + initialReportID: { + key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID, + }, }), )(AuthScreens); diff --git a/src/libs/Navigation/linkingConfig.js b/src/libs/Navigation/linkingConfig.js index b24e916ab1ec..b3a3ee95d93c 100644 --- a/src/libs/Navigation/linkingConfig.js +++ b/src/libs/Navigation/linkingConfig.js @@ -11,7 +11,7 @@ export default { config: { screens: { Home: { - path: '', + path: ROUTES.HOME, initialRouteName: 'Report', screens: { // Report route From 0df407f548e3ce70b00fd0da9b3fd090aa5a0285 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 13 Apr 2021 12:31:26 +0300 Subject: [PATCH 5/8] refactor: AuthScreens address PR comments --- .../Navigation/AppNavigator/AuthScreens.js | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js index 9f13d9261105..23bfc9331564 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.js +++ b/src/libs/Navigation/AppNavigator/AuthScreens.js @@ -75,9 +75,7 @@ class AuthScreens extends React.Component { Timing.start(CONST.TIMING.HOMEPAGE_INITIAL_RENDER); Timing.start(CONST.TIMING.HOMEPAGE_REPORTS_LOADED); - if (props.initialReportID) { - this.setInitialHomeParams(props.initialReportID); - } + this.initialReportID = props.initialReportID; } componentDidMount() { @@ -123,11 +121,11 @@ class AuthScreens extends React.Component { return true; } - // Update initialHomeParams only once - if (!this.initialHomeParams) { + // Skip when `this.initialReportID` is already assigned. We no longer want to update it + if (!this.initialReportID) { // Either we have a reportID or fetchAllReports resolved with no reports. Otherwise keep waiting if (nextProps.initialReportID || nextProps.initialReportID === '') { - this.setInitialHomeParams(nextProps.initialReportID); + this.initialReportID = nextProps.initialReportID; return true; } } @@ -142,21 +140,11 @@ class AuthScreens extends React.Component { this.interval = null; } - /** - * Setting the initial params would update the URL in the address bar to correctly - * It would also setup correct initial state for the Home screen - * - * @param {String} reportID - */ - setInitialHomeParams(reportID) { - this.initialHomeParams = { - screen: 'Report', - params: {reportID}, - }; - } - render() { - if (!this.initialHomeParams) { return null; } + // Wait to resolve initial Home route params. + if (!this.initialReportID) { + return null; + } const modalScreenOptions = { headerShown: false, @@ -180,7 +168,12 @@ class AuthScreens extends React.Component { headerShown: false, title: 'Expensify.cash', }} - initialParams={this.initialHomeParams} + initialParams={{ + screen: 'Report', + params: { + reportID: this.initialReportID, + }, + }} component={MainDrawerNavigator} /> From 5311e74f785e52f24616307f00ffe5cfdf9a84db Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 13 Apr 2021 13:25:43 +0300 Subject: [PATCH 6/8] refactor: NavigationRoot address PR comments --- src/libs/Navigation/NavigationRoot.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/Navigation/NavigationRoot.js b/src/libs/Navigation/NavigationRoot.js index 0b6707b70a0f..e88bdbafd344 100644 --- a/src/libs/Navigation/NavigationRoot.js +++ b/src/libs/Navigation/NavigationRoot.js @@ -19,14 +19,14 @@ class NavigationRoot extends Component { constructor(props) { super(props); - this.handleStateChange = this.handleStateChange.bind(this); + this.parseAndStoreRoute = this.parseAndStoreRoute.bind(this); } /** * Intercept state changes and perform different logic * @param {NavigationState} state */ - handleStateChange(state) { + parseAndStoreRoute(state) { if (!state) { return; } @@ -46,7 +46,7 @@ class NavigationRoot extends Component { return ( } - onStateChange={this.handleStateChange} + onStateChange={this.parseAndStoreRoute} ref={navigationRef} linking={linkingConfig} documentTitle={{ From afd5a068107a6d889cdf7fe851060b177231dcb4 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 13 Apr 2021 13:39:58 +0300 Subject: [PATCH 7/8] refactor: NavigationRoot move viewed report persistence logic The Report Screen renders the "currently viewed report" - it can persist it This way we doesn't have to check per each navigation state change --- src/libs/Navigation/NavigationRoot.js | 10 ---------- src/pages/home/ReportScreen.js | 7 +++++++ 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/libs/Navigation/NavigationRoot.js b/src/libs/Navigation/NavigationRoot.js index e88bdbafd344..322b7036914f 100644 --- a/src/libs/Navigation/NavigationRoot.js +++ b/src/libs/Navigation/NavigationRoot.js @@ -1,12 +1,9 @@ -import _ from 'underscore'; import React, {Component} from 'react'; import PropTypes from 'prop-types'; import {getPathFromState, NavigationContainer} from '@react-navigation/native'; import {navigationRef} from './Navigation'; import linkingConfig from './linkingConfig'; import AppNavigator from './AppNavigator'; -import ROUTES from '../../ROUTES'; -import {updateCurrentlyViewedReportID} from '../actions/Report'; import {setCurrentURL} from '../actions/App'; import FullScreenLoadingIndicator from '../../components/FullscreenLoadingIndicator'; @@ -32,13 +29,6 @@ class NavigationRoot extends Component { } const path = getPathFromState(state, linkingConfig.config); - if (path.includes(ROUTES.REPORT)) { - const reportID = Number(_.last(path.split('/'))); - if (reportID && !_.isNaN(reportID)) { - updateCurrentlyViewedReportID(reportID); - } - } - setCurrentURL(path); } diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 0438c2f6b910..7bc03a2492fc 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -7,6 +7,7 @@ import HeaderView from './HeaderView'; import Navigation from '../../libs/Navigation/Navigation'; import ROUTES from '../../ROUTES'; import FullScreenLoadingIndicator from '../../components/FullscreenLoadingIndicator'; +import {updateCurrentlyViewedReportID} from '../../libs/actions/Report'; const propTypes = { /* Navigation route context info provided by react navigation */ @@ -37,6 +38,7 @@ class ReportScreen extends React.Component { if (reportChanged) { this.prepareTransition(); + this.storeCurrentlyViewedReport(); } } @@ -73,6 +75,11 @@ class ReportScreen extends React.Component { this.loadingTimerId = setTimeout(() => this.setState({isLoading: false}), 300); } + storeCurrentlyViewedReport() { + const reportID = this.getReportID(); + updateCurrentlyViewedReportID(reportID); + } + render() { return ( From e6ef81fd984e61a4c2cf4b69eaa47f9d439f33c2 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 13 Apr 2021 20:49:40 +0300 Subject: [PATCH 8/8] docs: ReportScreen add method documentation --- src/pages/home/ReportScreen.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 7bc03a2492fc..ddbb72a9e659 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -75,6 +75,9 @@ class ReportScreen extends React.Component { this.loadingTimerId = setTimeout(() => this.setState({isLoading: false}), 300); } + /** + * Persists the currently viewed report id + */ storeCurrentlyViewedReport() { const reportID = this.getReportID(); updateCurrentlyViewedReportID(reportID);