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

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

Merged
merged 9 commits into from
Apr 13, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d1a2aa5
refactor: NavigationRoot extract `renderFallback`
kidroca Apr 12, 2021
ceac113
refactor: NavigationRoot extract change handler
kidroca Apr 12, 2021
0c53948
refactor: NavigationRoot remove Linking handling
kidroca Apr 12, 2021
d47683e
feat: AuthScreens setup initial Home screen params
kidroca Apr 12, 2021
0df407f
refactor: AuthScreens address PR comments
kidroca Apr 13, 2021
5311e74
refactor: NavigationRoot address PR comments
kidroca Apr 13, 2021
afd5a06
refactor: NavigationRoot move viewed report persistence logic
kidroca Apr 13, 2021
b6872ae
Merge remote-tracking branch 'e.cash/master' into kidroca/fix-url-not…
kidroca Apr 13, 2021
e6ef81f
docs: ReportScreen add method documentation
kidroca Apr 13, 2021
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
34 changes: 32 additions & 2 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
10 changes: 10 additions & 0 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
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,14 @@ class ReportScreen extends React.Component {
this.loadingTimerId = setTimeout(() => this.setState({isLoading: false}), 300);
}

/**
* Persists the currently viewed report id
*/
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