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

Fix: Address bar URL is not updated after login or landing on root #2346

34 changes: 32 additions & 2 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,16 @@ const propTypes = {
// Is the network currently offline or not
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 {
Expand All @@ -94,6 +99,8 @@ class AuthScreens extends React.Component {

Timing.start(CONST.TIMING.HOMEPAGE_INITIAL_RENDER);
Timing.start(CONST.TIMING.HOMEPAGE_REPORTS_LOADED);

this.initialReportID = props.initialReportID;
}

componentDidMount() {
Expand Down Expand Up @@ -134,11 +141,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;
}

// 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 === '') {
kidroca marked this conversation as resolved.
Show resolved Hide resolved
this.initialReportID = nextProps.initialReportID;
return true;
}
}

return false;
}

Expand All @@ -150,6 +166,11 @@ class AuthScreens extends React.Component {
}

render() {
// Wait to resolve initial Home route params.
if (!this.initialReportID) {
return null;
}

const modalScreenOptions = {
headerShown: false,
cardStyle: getNavigationModalCardStyle(this.props.isSmallScreenWidth),
Expand All @@ -172,6 +193,12 @@ class AuthScreens extends React.Component {
headerShown: false,
title: 'Expensify.cash',
}}
initialParams={{
screen: 'Report',
params: {
reportID: this.initialReportID,
},
}}
component={MainDrawerNavigator}
/>
<RootStack.Screen
Expand Down Expand Up @@ -243,5 +270,8 @@ export default compose(
network: {
key: ONYXKEYS.NETWORK,
},
initialReportID: {
key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
},
}),
)(AuthScreens);
110 changes: 16 additions & 94 deletions src/libs/Navigation/NavigationRoot.js
Original file line number Diff line number Diff line change
@@ -1,115 +1,42 @@
import _ from 'underscore';
import React, {Component} from 'react';
import {ActivityIndicator, Linking, View} 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 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
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.parseAndStoreRoute = this.parseAndStoreRoute.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);
kidroca marked this conversation as resolved.
Show resolved Hide resolved

// 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,
];
}
kidroca marked this conversation as resolved.
Show resolved Hide resolved
/**
* Intercept state changes and perform different logic
* @param {NavigationState} state
*/
parseAndStoreRoute(state) {
if (!state) {
return;
}

this.setState({loading: false, initialState});
});
const path = getPathFromState(state, linkingConfig.config);
setCurrentURL(path);
}

render() {
if (this.state.loading) {
return (
<View style={[styles.flex1, styles.h100, styles.justifyContentCenter]}>
<ActivityIndicator size="large" color={themeColors.spinner} />
</View>
);
}

// If we are on web, desktop, or a widescreen width we will use our custom navigator to create the wider layout
return (
<NavigationContainer
initialState={this.state.initialState}
onStateChange={(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);
}}
kidroca marked this conversation as resolved.
Show resolved Hide resolved
fallback={<FullScreenLoadingIndicator visible />}
onStateChange={this.parseAndStoreRoute}
ref={navigationRef}
linking={linkingConfig}
documentTitle={{
Expand All @@ -123,9 +50,4 @@ class NavigationRoot extends Component {
}

NavigationRoot.propTypes = propTypes;
NavigationRoot.defaultProps = defaultProps;
export default withOnyx({
currentlyViewedReportID: {
key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
},
})(NavigationRoot);
export default NavigationRoot;
2 changes: 1 addition & 1 deletion src/libs/Navigation/linkingConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default {
config: {
screens: {
Home: {
path: '',
path: ROUTES.HOME,
initialRouteName: 'Report',
screens: {
// Report route
Expand Down
7 changes: 7 additions & 0 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -37,6 +38,7 @@ class ReportScreen extends React.Component {

if (reportChanged) {
this.prepareTransition();
this.storeCurrentlyViewedReport();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replaces the logic here:

https://github.com/Expensify/Expensify.cash/blob/d3f1d28478357e2a66b155e660def191d6ed79a8/src/libs/Navigation/NavigationRoot.js#L104-L109

The ReportScreen renders the last viewed report so we don't need to check per each navigation change, but only save the last report that was viewed in ReportScreen

Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes sense to me. I don't see how it's different from what we had before (maybe a bit cleaner). Was there any other motivation for the change or just cleaning stuff up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Primarily clean up, it should still work the same as before. I can revert this you think it might cause some issue down the road

}
}

Expand Down Expand Up @@ -73,6 +75,11 @@ class ReportScreen extends React.Component {
this.loadingTimerId = setTimeout(() => this.setState({isLoading: false}), 300);
}

storeCurrentlyViewedReport() {
Copy link
Contributor

Choose a reason for hiding this comment

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

All our components methods must have a method doc even if they don't take params or return anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoid docs that don't add any additional information.
Method descriptions are optional and should be added when it's not obvious what the purpose of the method is.
https://github.com/Expensify/Expensify.cash/blob/master/STYLE.md#jsdocs 🤓

This is a class method and I'll just be adding this as a description: "Persists the currently viewed report id". I could extend it to "...with Onyx" but this is just an implementation detail

Anyway this is not the first time, someone would bring up func/method documentation. In fact it is always the case that I should add documentation so you might consider updating the guide - I would have just always added documentation if I hadn't read the above in the style guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for calling that out. I will update it to be more clear.

const reportID = this.getReportID();
updateCurrentlyViewedReportID(reportID);
kidroca marked this conversation as resolved.
Show resolved Hide resolved
}

render() {
return (
<ScreenWrapper style={[styles.appContent, styles.flex1]}>
Expand Down