From 845460de1680ba937032fe7835fe88727746ad6c Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 15 Sep 2020 11:13:19 -0600 Subject: [PATCH 01/28] Optimize how many reports render at one time --- src/page/home/MainView.js | 66 ++++++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index f8bfc2acf4f05..84b9389afb8db 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -26,24 +26,66 @@ const defaultProps = { reports: {}, }; -class MainView extends React.Component { - render() { - if (!_.size(this.props.reports)) { - return null; +class MainView extends React.PureComponent { + constructor(props) { + super(props); + this.areFirstReportsRendered = false; + this.areAllReportsRendered = false; + this.reportsToRender = null; + } + + /** + * This is an optimized method for rendering reports so that not too many + * things are rendered on the first load of the page + */ + componentDidUpdate() { + // If all reports have been rendered, that's good, there is nothing additional to do + if (this.areAllReportsRendered) { + return; + } + + // The first reports that get rendered are the ones that are unread, pinned, or matching the URL + if (!this.areFirstReportsRendered) { + this.areFirstReportsRendered = true; + this.reportsToRender = _.filter(this.props.reports, report => report.isUnread || report.pinnedReport || this.isReportIDMatchingURL(report.reportID)); + this.forceUpdate(); + return; + } + + // After the first reports are rendered, then the rest of the reports are rendered + // after a brief delay to give the UI thread some breathing room + if (!this.areAllReportsRendered) { + this.areAllReportsRendered = true; + this.reportsToRender = this.props.reports; + setTimeout(() => this.forceUpdate(), 5000); } + } + /** + * Looks to see if the reportID matches the report ID in the URL because that + * report needs to be the one that is visible while all the other reports are hidden + * + * @param {number} reportID + * + * @returns {boolean} + */ + isReportIDMatchingURL(reportID) { const reportIDInURL = parseInt(this.props.match.params.reportID, 10); + return reportID === reportIDInURL; + } + + render() { + if (!_.size(this.reportsToRender)) { + return null; + } - // The styles for each of our reports. Basically, they are all hidden except for the one matching the + // The styles for each of our report views. Basically, they are all hidden except for the one matching the // reportID in the URL - let activeReportID; - const reportStyles = _.reduce(this.props.reports, (memo, report) => { - const isActiveReport = reportIDInURL === report.reportID; + const reportStyles = _.reduce(this.reportsToRender, (memo, report) => { const finalData = {...memo}; let reportStyle; - if (isActiveReport) { - activeReportID = report.reportID; + if (this.isReportIDMatchingURL(report.reportID)) { reportStyle = [styles.dFlex, styles.flex1]; } else { reportStyle = [styles.dNone]; @@ -55,14 +97,14 @@ class MainView extends React.Component { return ( <> - {_.map(this.props.reports, report => ( + {_.map(this.reportsToRender, report => report.reportName && ( ))} From f447382f8d143d6ede0d02fa7462042dc6d86539 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 15 Sep 2020 11:15:12 -0600 Subject: [PATCH 02/28] Add some comments to explain the pure component --- src/page/home/MainView.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 84b9389afb8db..7b355c68f6081 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -26,6 +26,11 @@ const defaultProps = { reports: {}, }; +// This is a PureComponent because since this method is connected to an Ion collection, +// it has setState() called on it anytime a single report in the collection changes. When +// reports are first fetched, this component is rendered n times (where n is the number of reports). +// By switching to a PureComponent, it will only re-render if the props change which is +// much more performant. class MainView extends React.PureComponent { constructor(props) { super(props); From fbab50d9cc272869941767459a854da5c562519a Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 15 Sep 2020 11:27:31 -0600 Subject: [PATCH 03/28] Optimize the rendering of report actions --- src/page/home/report/ReportActionsView.js | 32 ++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 9271e3e64731e..db91384d54f74 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -35,6 +35,10 @@ class ReportActionsView extends React.Component { constructor(props) { super(props); + this.areFirstActionsRendered = false; + this.areAllActionsRendered = false; + this.actionsToRender = null; + this.scrollToListBottom = this.scrollToListBottom.bind(this); this.recordMaxAction = this.recordMaxAction.bind(this); } @@ -52,6 +56,28 @@ class ReportActionsView extends React.Component { if (isReportVisible && _.size(prevProps.reportActions) !== _.size(this.props.reportActions)) { setTimeout(this.recordMaxAction, 3000); } + + // If all report actions have been rendered, that's good, there is nothing additional to do + if (this.areAllActionsRendered) { + return; + } + + // Render the first 100 (most recent) report actions first + if (!this.areFirstActionsRendered) { + this.areFirstActionsRendered = true; + const sortedReportActions = _.sortBy(this.props.reportActions, 'sequenceNumber'); + this.actionsToRender = _.first(sortedReportActions, 5); + this.forceUpdate(); + return; + } + + // After the first reports are rendered, then the rest of the reports are rendered + // after a brief delay to give the UI thread some breathing room + if (!this.areAllActionsRendered) { + this.areAllActionsRendered = true; + this.actionsToRender = _.sortBy(this.props.reportActions, 'sequenceNumber'); + setTimeout(() => this.forceUpdate(), 5000); + } } componentWillUnmount() { @@ -127,7 +153,7 @@ class ReportActionsView extends React.Component { } render() { - if (!_.size(this.props.reportActions)) { + if (!_.size(this.actionsToRender)) { return ( Be the first person to comment! @@ -144,13 +170,13 @@ class ReportActionsView extends React.Component { bounces={false} contentContainerStyle={[styles.chatContentScrollView]} > - {_.chain(this.props.reportActions).sortBy('sequenceNumber').map((item, index) => ( + {_.map(this.actionsToRender, (item, index) => ( - )).value()} + ))} ); } From 8fa9a37c688679aeb3b774b50d58d31d628dd420 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 15 Sep 2020 16:44:44 -0600 Subject: [PATCH 04/28] Add HOC for batched rendering --- src/components/withBatchedRendering.js | 69 +++++++++++++++++++++++ src/page/home/MainView.js | 62 ++++++++------------ src/page/home/report/ReportActionsView.js | 56 ++++++++---------- 3 files changed, 115 insertions(+), 72 deletions(-) create mode 100644 src/components/withBatchedRendering.js diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js new file mode 100644 index 0000000000000..ec95384fdcd33 --- /dev/null +++ b/src/components/withBatchedRendering.js @@ -0,0 +1,69 @@ +/** + * This HOC is used to incrementally display a collection of data in batches. + * Example: in MainView.js only the visible reports should be rendered in the first batch, + * then all other reports are rendered in the second batch + */ +import React from 'react'; + +/** + * Returns the display name of a component + * + * @param {object} component + * @returns {string} + */ +function getDisplayName(component) { + return component.displayName || component.name || 'Component'; +} + +export default function (batchCallback, batchesToRender = 2, batchRenderDelay = 5000) { + return (WrappedComponent) => { + class withBatchedRendering extends React.Component { + constructor(props) { + super(props); + + this.batchesRendered = 0; + + this.state = { + itemsToRender: null, + }; + } + + componentDidUpdate() { + if (this.batchesRendered < batchesToRender) { + const renderDelay = this.batchesRendered === 0 + ? 0 + : batchRenderDelay; + const items = batchCallback(this.batchesRendered, this.props); + + // If the batchCallback returned false, then the props to get the items + // from didn't exist from Ion yet + if (items === false) { + return; + } + + setTimeout(() => { + this.setState({ + itemsToRender: items, + }); + }, renderDelay); + this.batchesRendered++; + } + } + + render() { + // Spreading props and state is necessary in an HOC where the data cannot be predicted + return ( + + ); + } + } + + withBatchedRendering.displayName = `withBatchedRendering(${getDisplayName(WrappedComponent)})`; + return withBatchedRendering; + }; +} diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 7b355c68f6081..d37675749b4a5 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -8,6 +8,7 @@ import IONKEYS from '../../IONKEYS'; import styles from '../../style/StyleSheet'; import {withRouter} from '../../lib/Router'; import compose from '../../lib/compose'; +import withBatchedRendering from '../../components/withBatchedRendering'; const propTypes = { // This comes from withRouter @@ -17,13 +18,13 @@ const propTypes = { /* Ion Props */ // List of reports to display - reports: PropTypes.objectOf(PropTypes.shape({ + itemsToRender: PropTypes.arrayOf(PropTypes.shape({ reportID: PropTypes.number, })), }; const defaultProps = { - reports: {}, + itemsToRender: {}, }; // This is a PureComponent because since this method is connected to an Ion collection, @@ -32,40 +33,6 @@ const defaultProps = { // By switching to a PureComponent, it will only re-render if the props change which is // much more performant. class MainView extends React.PureComponent { - constructor(props) { - super(props); - this.areFirstReportsRendered = false; - this.areAllReportsRendered = false; - this.reportsToRender = null; - } - - /** - * This is an optimized method for rendering reports so that not too many - * things are rendered on the first load of the page - */ - componentDidUpdate() { - // If all reports have been rendered, that's good, there is nothing additional to do - if (this.areAllReportsRendered) { - return; - } - - // The first reports that get rendered are the ones that are unread, pinned, or matching the URL - if (!this.areFirstReportsRendered) { - this.areFirstReportsRendered = true; - this.reportsToRender = _.filter(this.props.reports, report => report.isUnread || report.pinnedReport || this.isReportIDMatchingURL(report.reportID)); - this.forceUpdate(); - return; - } - - // After the first reports are rendered, then the rest of the reports are rendered - // after a brief delay to give the UI thread some breathing room - if (!this.areAllReportsRendered) { - this.areAllReportsRendered = true; - this.reportsToRender = this.props.reports; - setTimeout(() => this.forceUpdate(), 5000); - } - } - /** * Looks to see if the reportID matches the report ID in the URL because that * report needs to be the one that is visible while all the other reports are hidden @@ -80,13 +47,13 @@ class MainView extends React.PureComponent { } render() { - if (!_.size(this.reportsToRender)) { + if (!_.size(this.props.itemsToRender)) { return null; } // The styles for each of our report views. Basically, they are all hidden except for the one matching the // reportID in the URL - const reportStyles = _.reduce(this.reportsToRender, (memo, report) => { + const reportStyles = _.reduce(this.props.itemsToRender, (memo, report) => { const finalData = {...memo}; let reportStyle; @@ -102,7 +69,7 @@ class MainView extends React.PureComponent { return ( <> - {_.map(this.reportsToRender, report => report.reportName && ( + {_.map(this.props.itemsToRender, report => report.reportName && ( { + if (!props.reports) { + return false; + } + + // The first reports that get rendered are the ones that are unread, pinned, or matching the URL + if (currentBatch === 0) { + const reportIDInURL = parseInt(props.match.params.reportID, 10); + + // eslint-disable-next-line max-len + return _.filter(props.reports, report => report.isUnread || report.pinnedReport || report.reportID === reportIDInURL); + } + + // After the first reports are rendered, then the rest of the reports are rendered + // after a brief delay to give the UI thread some breathing room + return _.values(props.reports); + }), )(MainView); diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index db91384d54f74..6e9c991ea5825 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -12,6 +12,7 @@ import styles from '../../../style/StyleSheet'; import {withRouter} from '../../../lib/Router'; import ReportActionPropTypes from './ReportActionPropTypes'; import compose from '../../../lib/compose'; +import withBatchedRendering from '../../../components/withBatchedRendering'; const propTypes = { // These are from withRouter @@ -24,21 +25,17 @@ const propTypes = { /* Ion Props */ // Array of report actions for this report - reportActions: PropTypes.PropTypes.objectOf(PropTypes.shape(ReportActionPropTypes)), + itemsToRender: PropTypes.PropTypes.arrayOf(PropTypes.shape(ReportActionPropTypes)), }; const defaultProps = { - reportActions: {}, + itemsToRender: {}, }; class ReportActionsView extends React.Component { constructor(props) { super(props); - this.areFirstActionsRendered = false; - this.areAllActionsRendered = false; - this.actionsToRender = null; - this.scrollToListBottom = this.scrollToListBottom.bind(this); this.recordMaxAction = this.recordMaxAction.bind(this); } @@ -53,31 +50,9 @@ class ReportActionsView extends React.Component { // When the number of actions change, wait three seconds, then record the max action // This will make the unread indicator go away if you receive comments in the same chat you're looking at - if (isReportVisible && _.size(prevProps.reportActions) !== _.size(this.props.reportActions)) { + if (isReportVisible && _.size(prevProps.itemsToRender) !== _.size(this.props.itemsToRender)) { setTimeout(this.recordMaxAction, 3000); } - - // If all report actions have been rendered, that's good, there is nothing additional to do - if (this.areAllActionsRendered) { - return; - } - - // Render the first 100 (most recent) report actions first - if (!this.areFirstActionsRendered) { - this.areFirstActionsRendered = true; - const sortedReportActions = _.sortBy(this.props.reportActions, 'sequenceNumber'); - this.actionsToRender = _.first(sortedReportActions, 5); - this.forceUpdate(); - return; - } - - // After the first reports are rendered, then the rest of the reports are rendered - // after a brief delay to give the UI thread some breathing room - if (!this.areAllActionsRendered) { - this.areAllActionsRendered = true; - this.actionsToRender = _.sortBy(this.props.reportActions, 'sequenceNumber'); - setTimeout(() => this.forceUpdate(), 5000); - } } componentWillUnmount() { @@ -97,7 +72,7 @@ class ReportActionsView extends React.Component { */ // eslint-disable-next-line isConsecutiveActionMadeByPreviousActor(actionIndex) { - const reportActions = lodashGet(this.props, 'reportActions', {}); + const reportActions = lodashGet(this.props, 'itemsToRender', {}); // This is the created action and the very first action so it cannot be a consecutive comment. if (actionIndex === 0) { @@ -131,7 +106,7 @@ class ReportActionsView extends React.Component { * action when scrolled */ recordMaxAction() { - const reportActions = lodashGet(this.props, 'reportActions', {}); + const reportActions = lodashGet(this.props, 'itemsToRender', {}); const maxVisibleSequenceNumber = _.chain(reportActions) .pluck('sequenceNumber') .max() @@ -153,7 +128,7 @@ class ReportActionsView extends React.Component { } render() { - if (!_.size(this.actionsToRender)) { + if (!_.size(this.props.itemsToRender)) { return ( Be the first person to comment! @@ -170,7 +145,7 @@ class ReportActionsView extends React.Component { bounces={false} contentContainerStyle={[styles.chatContentScrollView]} > - {_.map(this.actionsToRender, (item, index) => ( + {_.map(this.props.itemsToRender, (item, index) => ( { + if (!props.reportActions) { + return false; + } + + // Render the last 100 (most recent) report actions first + if (currentBatch === 0) { + const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); + return _.last(sortedReportActions, 100); + } + + // After the first reports are rendered, then the rest of the reports are rendered + // after a brief delay to give the UI thread some breathing room + return _.sortBy(_.values(props.reportActions), 'sequenceNumber'); + }) )(ReportActionsView); From 8ed7f31299b85a29fec2b33742d89597b43a4e7a Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Tue, 15 Sep 2020 16:58:32 -0600 Subject: [PATCH 05/28] Remove initial rendering logic --- src/components/withBatchedRendering.js | 6 ------ src/components/withIon.js | 4 ++-- src/page/home/MainView.js | 4 ---- src/page/home/report/ReportActionsView.js | 4 ---- 4 files changed, 2 insertions(+), 16 deletions(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index ec95384fdcd33..19c67001830a2 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -35,12 +35,6 @@ export default function (batchCallback, batchesToRender = 2, batchRenderDelay = : batchRenderDelay; const items = batchCallback(this.batchesRendered, this.props); - // If the batchCallback returned false, then the props to get the items - // from didn't exist from Ion yet - if (items === false) { - return; - } - setTimeout(() => { this.setState({ itemsToRender: items, diff --git a/src/components/withIon.js b/src/components/withIon.js index de47559a90e9d..cf1c44490fb7a 100644 --- a/src/components/withIon.js +++ b/src/components/withIon.js @@ -96,8 +96,8 @@ export default function (mapIonToState) { * Takes a single mapping and binds the state of the component to the store * * @param {object} mapping - * @param {string|function} mapping.key key to connect to. can be a string or a function that takes this.props - * as an argument and returns a string + * @param {string|function} mapping.key key to connect to. can be a string or a function that takes + * this.props as an argument and returns a string * @param {string} statePropertyName the name of the state property that Ion will add the data to * @param {string} [mapping.indexBy] the name of the ID property to use for the collection * @param {boolean} [mapping.initWithStoredValues] If set to false, then no data will be prefilled into the diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index d37675749b4a5..1c8b4bb3c7a1f 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -97,10 +97,6 @@ export default compose( }, }), withBatchedRendering((currentBatch, props) => { - if (!props.reports) { - return false; - } - // The first reports that get rendered are the ones that are unread, pinned, or matching the URL if (currentBatch === 0) { const reportIDInURL = parseInt(props.match.params.reportID, 10); diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 8f6b4fe2a5e0c..55a5794003fd2 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -168,10 +168,6 @@ export default compose( }, }), withBatchedRendering((currentBatch, props) => { - if (!props.reportActions) { - return false; - } - // Render the last 100 (most recent) report actions first if (currentBatch === 0) { const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); From 8160cec642f518980c4360b99303ddc4a7c111bb Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 07:57:56 -0600 Subject: [PATCH 06/28] Improve comment --- src/page/home/MainView.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 08a8eae88dc49..dc84e988858b7 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -27,11 +27,12 @@ const defaultProps = { itemsToRender: {}, }; -// This is a PureComponent because since this method is connected to an Ion collection, -// it has setState() called on it anytime a single report in the collection changes. When -// reports are first fetched, this component is rendered n times (where n is the number of reports). -// By switching to a PureComponent, it will only re-render if the props change which is -// much more performant. +// Since this component is connected to an Ion collection, it's props are updated anytime a single report +// in the collection changes. +// When reports are first fetched, this component is rendered n times (where n is the number of reports). +// There are only two pieces of data this component cares about: +// - reportID comes from the collection and is used to render ReportView, this data will never change after first render +// - match.params.reportID comes from React Router and will change anytime a report is selected from LHN class MainView extends React.PureComponent { /** * Looks to see if the reportID matches the report ID in the URL because that From 5891973029162c3fe7a5ae9f62b79a12c4f719ca Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 08:00:49 -0600 Subject: [PATCH 07/28] Remove an extra view --- src/page/home/MainView.js | 11 ++++------- src/page/home/report/ReportView.js | 5 ++++- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index dc84e988858b7..0af880480e017 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -71,15 +71,12 @@ class MainView extends React.PureComponent { return ( <> {_.map(this.props.itemsToRender, report => report.reportName && ( - - - + reportID={report.reportID} + isActiveReport={this.isReportIDMatchingURL(report.reportID)} + /> ))} ); diff --git a/src/page/home/report/ReportView.js b/src/page/home/report/ReportView.js index 642afd1df8294..70c743cae5bd4 100644 --- a/src/page/home/report/ReportView.js +++ b/src/page/home/report/ReportView.js @@ -13,6 +13,9 @@ const propTypes = { // Whether or not this report is the one that is currently being viewed isActiveReport: PropTypes.bool.isRequired, + + // These are styles to apply to the report view + style: PropTypes.any.isRequired, }; // This is a PureComponent so that it only re-renders when the reportID changes or when the report changes from @@ -23,7 +26,7 @@ class ReportView extends React.PureComponent { // calling focus() on 42 different forms doesn't work const shouldShowComposeForm = this.props.isActiveReport; return ( - + {shouldShowComposeForm && ( From 901585a1ca267f28c7b2dee126de824556aa6d87 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 08:13:01 -0600 Subject: [PATCH 08/28] Improve the batch definitions --- src/components/withBatchedRendering.js | 22 ++++++++-------------- src/page/home/MainView.js | 22 ++++++++-------------- src/page/home/report/ReportActionsView.js | 18 +++++++----------- src/page/home/report/ReportView.js | 1 + 4 files changed, 24 insertions(+), 39 deletions(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index 19c67001830a2..4a45274f5f98f 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -4,6 +4,7 @@ * then all other reports are rendered in the second batch */ import React from 'react'; +import _ from 'underscore'; /** * Returns the display name of a component @@ -15,33 +16,26 @@ function getDisplayName(component) { return component.displayName || component.name || 'Component'; } -export default function (batchCallback, batchesToRender = 2, batchRenderDelay = 5000) { +export default function (batches) { return (WrappedComponent) => { class withBatchedRendering extends React.Component { constructor(props) { super(props); - this.batchesRendered = 0; - this.state = { itemsToRender: null, }; } - componentDidUpdate() { - if (this.batchesRendered < batchesToRender) { - const renderDelay = this.batchesRendered === 0 - ? 0 - : batchRenderDelay; - const items = batchCallback(this.batchesRendered, this.props); - + componentDidMount() { + console.log(this); + _.each(batches(this.props), (batch) => { setTimeout(() => { this.setState({ - itemsToRender: items, + itemsToRender: batch.items, }); - }, renderDelay); - this.batchesRendered++; - } + }, batch.delay || 0); + }); } render() { diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 0af880480e017..2e48963ac0230 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -1,5 +1,4 @@ import React from 'react'; -import {View} from 'react-native'; import PropTypes from 'prop-types'; import _ from 'underscore'; import ReportView from './report/ReportView'; @@ -18,7 +17,7 @@ const propTypes = { /* Ion Props */ // List of reports to display - itemsToRender: PropTypes.arrayOf(PropTypes.shape({ + itemsToRender: PropTypes.objectOf(PropTypes.shape({ reportID: PropTypes.number, })), }; @@ -93,17 +92,12 @@ export default compose( key: IONKEYS.COLLECTION.REPORT, }, }), - withBatchedRendering((currentBatch, props) => { - // The first reports that get rendered are the ones that are unread, pinned, or matching the URL - if (currentBatch === 0) { - const reportIDInURL = parseInt(props.match.params.reportID, 10); - - // eslint-disable-next-line max-len - return _.filter(props.reports, report => report.isUnread || report.pinnedReport || report.reportID === reportIDInURL); - } - - // After the first reports are rendered, then the rest of the reports are rendered - // after a brief delay to give the UI thread some breathing room - return _.values(props.reports); + withBatchedRendering((props) => { + const reportIDInURL = parseInt(props.match.params.reportID, 10); + const isReportVisible = report => report.isUnread || report.pinnedReport || report.reportID === reportIDInURL; + return [ + {items: _.pick(props.reports, isReportVisible), delay: 0}, + {items: props.reports, delay: 5000}, + ]; }), )(MainView); diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 2073e31d6f974..b803ebefe69b0 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -167,15 +167,11 @@ export default compose( key: ({reportID}) => `${IONKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, }, }), - withBatchedRendering((currentBatch, props) => { - // Render the last 100 (most recent) report actions first - if (currentBatch === 0) { - const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); - return _.last(sortedReportActions, 100); - } - - // After the first reports are rendered, then the rest of the reports are rendered - // after a brief delay to give the UI thread some breathing room - return _.sortBy(_.values(props.reportActions), 'sequenceNumber'); - }) + withBatchedRendering((props) => { + const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); + return [ + {items: _.last(sortedReportActions, 100), delay: 0}, + {items: sortedReportActions, delay: 5000}, + ]; + }), )(ReportActionsView); diff --git a/src/page/home/report/ReportView.js b/src/page/home/report/ReportView.js index 70c743cae5bd4..5d53ae7d671cd 100644 --- a/src/page/home/report/ReportView.js +++ b/src/page/home/report/ReportView.js @@ -15,6 +15,7 @@ const propTypes = { isActiveReport: PropTypes.bool.isRequired, // These are styles to apply to the report view + // eslint-disable-next-line react/forbid-prop-types style: PropTypes.any.isRequired, }; From f6672ddf992f2086629a16f41d7c20e3e61e1187 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 11:34:58 -0600 Subject: [PATCH 09/28] Improve props --- src/components/withBatchedRendering.js | 1 - src/page/home/MainView.js | 15 ++++++++---- src/page/home/report/ReportActionsView.js | 29 +++++++++++++---------- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index 4a45274f5f98f..0e0f0106998f2 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -28,7 +28,6 @@ export default function (batches) { } componentDidMount() { - console.log(this); _.each(batches(this.props), (batch) => { setTimeout(() => { this.setState({ diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 2e48963ac0230..2a182695f9def 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -2,11 +2,11 @@ import React from 'react'; import PropTypes from 'prop-types'; import _ from 'underscore'; import ReportView from './report/ReportView'; -import withIon from '../../components/withIon'; import IONKEYS from '../../IONKEYS'; import styles from '../../style/StyleSheet'; import {withRouter} from '../../lib/Router'; import compose from '../../lib/compose'; +import withIon from '../../components/withIon'; import withBatchedRendering from '../../components/withBatchedRendering'; const propTypes = { @@ -14,15 +14,22 @@ const propTypes = { // eslint-disable-next-line react/forbid-prop-types match: PropTypes.object.isRequired, - /* Ion Props */ + /* From withIon() */ + // The reports from Ion that exist and will have a view rendered for them + // eslint-disable-next-line react/no-unused-prop-types + reports: PropTypes.objectOf(PropTypes.shape({ + reportID: PropTypes.number, + })), - // List of reports to display - itemsToRender: PropTypes.objectOf(PropTypes.shape({ + /* From withBatchedRendering() */ + // The specific items that need to be rendered + itemsToRender: PropTypes.PropTypes.objectOf(PropTypes.shape({ reportID: PropTypes.number, })), }; const defaultProps = { + reports: {}, itemsToRender: {}, }; diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index b803ebefe69b0..a9ec37223fdd5 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -2,7 +2,6 @@ import React from 'react'; import {View, ScrollView, Keyboard} from 'react-native'; import PropTypes from 'prop-types'; import _ from 'underscore'; -import lodashGet from 'lodash.get'; import Text from '../../../components/Text'; import withIon from '../../../components/withIon'; import {fetchActions, updateLastReadActionID} from '../../../lib/actions/Report'; @@ -15,20 +14,24 @@ import compose from '../../../lib/compose'; import withBatchedRendering from '../../../components/withBatchedRendering'; const propTypes = { - // These are from withRouter - // eslint-disable-next-line react/forbid-prop-types - match: PropTypes.object.isRequired, - // The ID of the report actions will be created for reportID: PropTypes.number.isRequired, - /* Ion Props */ + /* From withRouter() */ + // eslint-disable-next-line react/forbid-prop-types + match: PropTypes.object.isRequired, + + /* From withIon() */ + // All of the report actions for this report + reportActions: PropTypes.PropTypes.objectOf(PropTypes.shape(ReportActionPropTypes)), - // Array of report actions for this report - itemsToRender: PropTypes.PropTypes.arrayOf(PropTypes.shape(ReportActionPropTypes)), + /* From withBatchedRendering() */ + // The specific items that need to be rendered + itemsToRender: PropTypes.PropTypes.objectOf(PropTypes.shape(ReportActionPropTypes)), }; const defaultProps = { + reportActions: {}, itemsToRender: {}, }; @@ -72,7 +75,7 @@ class ReportActionsView extends React.Component { */ // eslint-disable-next-line isConsecutiveActionMadeByPreviousActor(actionIndex) { - const reportActions = lodashGet(this.props, 'itemsToRender', {}); + const {reportActions} = this.props; // This is the created action and the very first action so it cannot be a consecutive comment. if (actionIndex === 0) { @@ -106,7 +109,7 @@ class ReportActionsView extends React.Component { * action when scrolled */ recordMaxAction() { - const reportActions = lodashGet(this.props, 'itemsToRender', {}); + const {reportActions} = this.props; const maxVisibleSequenceNumber = _.chain(reportActions) .pluck('sequenceNumber') .max() @@ -128,7 +131,7 @@ class ReportActionsView extends React.Component { } render() { - if (!_.size(this.props.itemsToRender)) { + if (!_.size(this.props.reportActions)) { return ( Be the first person to comment! @@ -170,8 +173,8 @@ export default compose( withBatchedRendering((props) => { const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); return [ - {items: _.last(sortedReportActions, 100), delay: 0}, - {items: sortedReportActions, delay: 5000}, + {items: _.chain(sortedReportActions).last(100).indexBy('reportID').value(), delay: 0}, + {items: _.indexBy(sortedReportActions, 'reportID'), delay: 7000}, ]; }), )(ReportActionsView); From 4f1a229e26ae9fb86b2661e667a4bc33e9631356 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 11:35:15 -0600 Subject: [PATCH 10/28] Rename comments to indicate where props are coming from --- src/Expensify.js | 2 +- src/page/SignInPage.js | 2 +- src/page/home/HeaderView.js | 2 +- src/page/home/sidebar/ChatSwitcherView.js | 2 +- src/page/home/sidebar/SidebarBottom.js | 2 +- src/page/home/sidebar/SidebarLink.js | 2 +- src/page/home/sidebar/SidebarLinks.js | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Expensify.js b/src/Expensify.js index e83e06622a758..adbb3fbe2a1e6 100644 --- a/src/Expensify.js +++ b/src/Expensify.js @@ -24,7 +24,7 @@ import ROUTES from './ROUTES'; Ion.init(); const propTypes = { - /* Ion Props */ + /* From withIon() */ // A route set by Ion that we will redirect to if present. Always empty on app init. redirectTo: PropTypes.string, diff --git a/src/page/SignInPage.js b/src/page/SignInPage.js index c989da8948df5..85696e19389b7 100644 --- a/src/page/SignInPage.js +++ b/src/page/SignInPage.js @@ -23,7 +23,7 @@ const propTypes = { // eslint-disable-next-line react/forbid-prop-types match: PropTypes.object.isRequired, - /* Ion Props */ + /* From withIon() */ // The session of the logged in person session: PropTypes.shape({ diff --git a/src/page/home/HeaderView.js b/src/page/home/HeaderView.js index 863e8b3e92b17..5fbab86c134f9 100644 --- a/src/page/home/HeaderView.js +++ b/src/page/home/HeaderView.js @@ -16,7 +16,7 @@ const propTypes = { // Decides whether we should show the hamburger menu button shouldShowHamburgerButton: PropTypes.bool.isRequired, - /* Ion Props */ + /* From withIon() */ // The report currently being looked at report: PropTypes.shape({ // Name of the report diff --git a/src/page/home/sidebar/ChatSwitcherView.js b/src/page/home/sidebar/ChatSwitcherView.js index 6d8527932a61e..f7d911c5b2a0f 100644 --- a/src/page/home/sidebar/ChatSwitcherView.js +++ b/src/page/home/sidebar/ChatSwitcherView.js @@ -41,7 +41,7 @@ const propTypes = { // A method that is triggered when the TextInput loses focus onBlur: PropTypes.func.isRequired, - /* Ion Props */ + /* From withIon() */ // All of the personal details for everyone // The keys of this object are the logins of the users, and the values are an object diff --git a/src/page/home/sidebar/SidebarBottom.js b/src/page/home/sidebar/SidebarBottom.js index e53f8beb790ba..2b51782e3687c 100644 --- a/src/page/home/sidebar/SidebarBottom.js +++ b/src/page/home/sidebar/SidebarBottom.js @@ -14,7 +14,7 @@ const propTypes = { // Safe area insets required for mobile devices margins insets: SafeAreaInsetPropTypes.isRequired, - /* Ion Props */ + /* From withIon() */ // The personal details of the person who is logged in myPersonalDetails: PropTypes.shape({ diff --git a/src/page/home/sidebar/SidebarLink.js b/src/page/home/sidebar/SidebarLink.js index c83f8fbb0bb89..a05a3ad0bbb82 100644 --- a/src/page/home/sidebar/SidebarLink.js +++ b/src/page/home/sidebar/SidebarLink.js @@ -23,7 +23,7 @@ const propTypes = { // Toggles the hamburger menu open and closed onLinkClick: PropTypes.func.isRequired, - /* Ion Props */ + /* From withIon() */ // The report object for this link report: PropTypes.shape({ diff --git a/src/page/home/sidebar/SidebarLinks.js b/src/page/home/sidebar/SidebarLinks.js index 2dd7c39881d2e..fbee127885d22 100644 --- a/src/page/home/sidebar/SidebarLinks.js +++ b/src/page/home/sidebar/SidebarLinks.js @@ -31,7 +31,7 @@ const propTypes = { // Safe area insets required for mobile devices margins insets: SafeAreaInsetPropTypes.isRequired, - /* Ion Props */ + /* From withIon() */ // List of reports reports: PropTypes.objectOf(PropTypes.shape({ From c0a3dedc7dc308522c5a7717fbb393607425e0db Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 11:39:01 -0600 Subject: [PATCH 11/28] Add some comments for how the HOC is used --- src/page/home/MainView.js | 4 ++++ src/page/home/report/ReportActionsView.js | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 2a182695f9def..1f4d612c7de72 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -99,6 +99,10 @@ export default compose( key: IONKEYS.COLLECTION.REPORT, }, }), + + // The rendering of report views is done in batches. + // The first batch are all the reports that are visible in the LHN. + // The second batch are all the reports. withBatchedRendering((props) => { const reportIDInURL = parseInt(props.match.params.reportID, 10); const isReportVisible = report => report.isUnread || report.pinnedReport || report.reportID === reportIDInURL; diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index a9ec37223fdd5..1c6df238c915f 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -170,6 +170,10 @@ export default compose( key: ({reportID}) => `${IONKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, }, }), + + // The rendering of report actions happens in batches. + // The first batch of actions is limited to the 100 most recent actions. + // The second batch is all of the rest of the actions. withBatchedRendering((props) => { const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); return [ From 3f77ea9252cd0a40fb1c4931b31dadfcb97d67e0 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 11:59:03 -0600 Subject: [PATCH 12/28] Refactor URL storage in Ion a little --- src/Expensify.js | 2 +- src/IONKEYS.js | 5 ++++- src/lib/actions/App.js | 4 +++- src/lib/actions/Report.js | 15 ++++++++++----- src/lib/actions/SignInRedirect.js | 2 +- src/page/home/MainView.js | 8 ++++---- src/page/home/report/ReportView.js | 9 +++------ src/page/home/sidebar/SidebarLinks.js | 4 ++-- 8 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/Expensify.js b/src/Expensify.js index adbb3fbe2a1e6..a330fdcc29715 100644 --- a/src/Expensify.js +++ b/src/Expensify.js @@ -85,7 +85,7 @@ class Expensify extends Component { {/* If there is ever a property for redirecting, we do the redirect here */} {/* Leave this as a ternary or else iOS throws an error about text not being wrapped in */} {redirectTo ? : null} - + currentURL = val, }); +let currentReportIDInURL; +Ion.connect({ + key: IONKEYS.URL.PARAMS, + callback: val => currentReportIDInURL = parseInt(val.reportID, 10), +}); + let personalDetails; Ion.connect({ key: IONKEYS.PERSONAL_DETAILS, @@ -95,8 +101,9 @@ function getSimplifiedReportObject(report) { reportID: report.reportID, reportName: report.reportName, reportNameValuePairs: report.reportNameValuePairs, + isReportIDInURL: currentReportIDInURL === report.reportID, isUnread: hasUnreadActions(report), - pinnedReport: configReportIDs.includes(report.reportID), + isPinned: configReportIDs.includes(report.reportID), }; } @@ -197,10 +204,8 @@ function updateReportWithNewAction(reportID, reportAction) { return; } - const currentReportID = Number(lodashGet(currentURL.split('/'), [1], 0)); - // If we are currently viewing this report do not show a notification. - if (reportID === currentReportID) { + if (reportID === currentReportIDInURL) { console.debug('[NOTIFICATION] No notification because it was a comment for the current report'); return; } diff --git a/src/lib/actions/SignInRedirect.js b/src/lib/actions/SignInRedirect.js index 6f0715a5d73cc..3bf70d409754f 100644 --- a/src/lib/actions/SignInRedirect.js +++ b/src/lib/actions/SignInRedirect.js @@ -5,7 +5,7 @@ import {redirect} from './App'; let currentURL; Ion.connect({ - key: IONKEYS.CURRENT_URL, + key: IONKEYS.URL.CURRENT, callback: val => currentURL = val, }); diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 1f4d612c7de72..228f4c158655c 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -24,7 +24,8 @@ const propTypes = { /* From withBatchedRendering() */ // The specific items that need to be rendered itemsToRender: PropTypes.PropTypes.objectOf(PropTypes.shape({ - reportID: PropTypes.number, + reportID: PropTypes.number.isRequired, + isReportIDInURL: PropTypes.bool, })), }; @@ -81,7 +82,7 @@ class MainView extends React.PureComponent { key={report.reportID} style={reportStyles[report.reportID]} reportID={report.reportID} - isActiveReport={this.isReportIDMatchingURL(report.reportID)} + showComposeForm={report.isReportIDInURL || false} /> ))} @@ -104,8 +105,7 @@ export default compose( // The first batch are all the reports that are visible in the LHN. // The second batch are all the reports. withBatchedRendering((props) => { - const reportIDInURL = parseInt(props.match.params.reportID, 10); - const isReportVisible = report => report.isUnread || report.pinnedReport || report.reportID === reportIDInURL; + const isReportVisible = report => report.isUnread || report.isPinned || report.isReportIDInURL; return [ {items: _.pick(props.reports, isReportVisible), delay: 0}, {items: props.reports, delay: 5000}, diff --git a/src/page/home/report/ReportView.js b/src/page/home/report/ReportView.js index 5d53ae7d671cd..77c9db37404ef 100644 --- a/src/page/home/report/ReportView.js +++ b/src/page/home/report/ReportView.js @@ -11,8 +11,8 @@ const propTypes = { // The ID of the report actions will be created for reportID: PropTypes.number.isRequired, - // Whether or not this report is the one that is currently being viewed - isActiveReport: PropTypes.bool.isRequired, + // Whether or not to show the compose comment form + showComposeForm: PropTypes.bool.isRequired, // These are styles to apply to the report view // eslint-disable-next-line react/forbid-prop-types @@ -23,14 +23,11 @@ const propTypes = { // active to inactive (or vice versa). This should greatly reduce how often comments are re-rendered. class ReportView extends React.PureComponent { render() { - // Only display the compose form for the active report because the form needs to get focus and - // calling focus() on 42 different forms doesn't work - const shouldShowComposeForm = this.props.isActiveReport; return ( - {shouldShowComposeForm && ( + {this.props.showComposeForm && ( addAction(this.props.reportID, text)} reportID={this.props.reportID} diff --git a/src/page/home/sidebar/SidebarLinks.js b/src/page/home/sidebar/SidebarLinks.js index fbee127885d22..2610166b6a854 100644 --- a/src/page/home/sidebar/SidebarLinks.js +++ b/src/page/home/sidebar/SidebarLinks.js @@ -56,11 +56,11 @@ class SidebarLinks extends React.Component { render() { const {reports, onLinkClick} = this.props; const reportIDInUrl = parseInt(this.props.match.params.reportID, 10); - const sortedReports = lodashOrderby(this.props.reports, ['pinnedReport', 'reportName'], ['desc', 'asc']); + const sortedReports = lodashOrderby(this.props.reports, ['isPinned', 'reportName'], ['desc', 'asc']); // Filter the reports so that the only reports shown are pinned, unread, and the one matching the URL // eslint-disable-next-line max-len - const reportsToDisplay = _.filter(sortedReports, report => (report.pinnedReport || report.isUnread || report.reportID === reportIDInUrl)); + const reportsToDisplay = _.filter(sortedReports, report => (report.isPinned || report.isUnread || report.reportID === reportIDInUrl)); // Updates the page title to indicate there are unread reports PageTitleUpdater(_.any(reports, report => report.isUnread)); From 6293c5846e5901a759d006c3d153b0416240c51f Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 12:19:41 -0600 Subject: [PATCH 13/28] Revert "Refactor URL storage in Ion a little" This reverts commit 3f77ea9252cd0a40fb1c4931b31dadfcb97d67e0. --- src/Expensify.js | 2 +- src/IONKEYS.js | 5 +---- src/lib/actions/App.js | 4 +--- src/lib/actions/Report.js | 15 +++++---------- src/lib/actions/SignInRedirect.js | 2 +- src/page/home/MainView.js | 8 ++++---- src/page/home/report/ReportView.js | 9 ++++++--- src/page/home/sidebar/SidebarLinks.js | 4 ++-- 8 files changed, 21 insertions(+), 28 deletions(-) diff --git a/src/Expensify.js b/src/Expensify.js index a330fdcc29715..adbb3fbe2a1e6 100644 --- a/src/Expensify.js +++ b/src/Expensify.js @@ -85,7 +85,7 @@ class Expensify extends Component { {/* If there is ever a property for redirecting, we do the redirect here */} {/* Leave this as a ternary or else iOS throws an error about text not being wrapped in */} {redirectTo ? : null} - + currentURL = val, }); -let currentReportIDInURL; -Ion.connect({ - key: IONKEYS.URL.PARAMS, - callback: val => currentReportIDInURL = parseInt(val.reportID, 10), -}); - let personalDetails; Ion.connect({ key: IONKEYS.PERSONAL_DETAILS, @@ -101,9 +95,8 @@ function getSimplifiedReportObject(report) { reportID: report.reportID, reportName: report.reportName, reportNameValuePairs: report.reportNameValuePairs, - isReportIDInURL: currentReportIDInURL === report.reportID, isUnread: hasUnreadActions(report), - isPinned: configReportIDs.includes(report.reportID), + pinnedReport: configReportIDs.includes(report.reportID), }; } @@ -204,8 +197,10 @@ function updateReportWithNewAction(reportID, reportAction) { return; } + const currentReportID = Number(lodashGet(currentURL.split('/'), [1], 0)); + // If we are currently viewing this report do not show a notification. - if (reportID === currentReportIDInURL) { + if (reportID === currentReportID) { console.debug('[NOTIFICATION] No notification because it was a comment for the current report'); return; } diff --git a/src/lib/actions/SignInRedirect.js b/src/lib/actions/SignInRedirect.js index 3bf70d409754f..6f0715a5d73cc 100644 --- a/src/lib/actions/SignInRedirect.js +++ b/src/lib/actions/SignInRedirect.js @@ -5,7 +5,7 @@ import {redirect} from './App'; let currentURL; Ion.connect({ - key: IONKEYS.URL.CURRENT, + key: IONKEYS.CURRENT_URL, callback: val => currentURL = val, }); diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 228f4c158655c..1f4d612c7de72 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -24,8 +24,7 @@ const propTypes = { /* From withBatchedRendering() */ // The specific items that need to be rendered itemsToRender: PropTypes.PropTypes.objectOf(PropTypes.shape({ - reportID: PropTypes.number.isRequired, - isReportIDInURL: PropTypes.bool, + reportID: PropTypes.number, })), }; @@ -82,7 +81,7 @@ class MainView extends React.PureComponent { key={report.reportID} style={reportStyles[report.reportID]} reportID={report.reportID} - showComposeForm={report.isReportIDInURL || false} + isActiveReport={this.isReportIDMatchingURL(report.reportID)} /> ))} @@ -105,7 +104,8 @@ export default compose( // The first batch are all the reports that are visible in the LHN. // The second batch are all the reports. withBatchedRendering((props) => { - const isReportVisible = report => report.isUnread || report.isPinned || report.isReportIDInURL; + const reportIDInURL = parseInt(props.match.params.reportID, 10); + const isReportVisible = report => report.isUnread || report.pinnedReport || report.reportID === reportIDInURL; return [ {items: _.pick(props.reports, isReportVisible), delay: 0}, {items: props.reports, delay: 5000}, diff --git a/src/page/home/report/ReportView.js b/src/page/home/report/ReportView.js index 77c9db37404ef..5d53ae7d671cd 100644 --- a/src/page/home/report/ReportView.js +++ b/src/page/home/report/ReportView.js @@ -11,8 +11,8 @@ const propTypes = { // The ID of the report actions will be created for reportID: PropTypes.number.isRequired, - // Whether or not to show the compose comment form - showComposeForm: PropTypes.bool.isRequired, + // Whether or not this report is the one that is currently being viewed + isActiveReport: PropTypes.bool.isRequired, // These are styles to apply to the report view // eslint-disable-next-line react/forbid-prop-types @@ -23,11 +23,14 @@ const propTypes = { // active to inactive (or vice versa). This should greatly reduce how often comments are re-rendered. class ReportView extends React.PureComponent { render() { + // Only display the compose form for the active report because the form needs to get focus and + // calling focus() on 42 different forms doesn't work + const shouldShowComposeForm = this.props.isActiveReport; return ( - {this.props.showComposeForm && ( + {shouldShowComposeForm && ( addAction(this.props.reportID, text)} reportID={this.props.reportID} diff --git a/src/page/home/sidebar/SidebarLinks.js b/src/page/home/sidebar/SidebarLinks.js index 2610166b6a854..fbee127885d22 100644 --- a/src/page/home/sidebar/SidebarLinks.js +++ b/src/page/home/sidebar/SidebarLinks.js @@ -56,11 +56,11 @@ class SidebarLinks extends React.Component { render() { const {reports, onLinkClick} = this.props; const reportIDInUrl = parseInt(this.props.match.params.reportID, 10); - const sortedReports = lodashOrderby(this.props.reports, ['isPinned', 'reportName'], ['desc', 'asc']); + const sortedReports = lodashOrderby(this.props.reports, ['pinnedReport', 'reportName'], ['desc', 'asc']); // Filter the reports so that the only reports shown are pinned, unread, and the one matching the URL // eslint-disable-next-line max-len - const reportsToDisplay = _.filter(sortedReports, report => (report.isPinned || report.isUnread || report.reportID === reportIDInUrl)); + const reportsToDisplay = _.filter(sortedReports, report => (report.pinnedReport || report.isUnread || report.reportID === reportIDInUrl)); // Updates the page title to indicate there are unread reports PageTitleUpdater(_.any(reports, report => report.isUnread)); From 41fa8a1b083db81bc722a94a1a4495eec737f013 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Wed, 16 Sep 2020 12:19:52 -0600 Subject: [PATCH 14/28] Fix indexing --- src/page/home/report/ReportActionsView.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 1c6df238c915f..b904bcc131bc8 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -177,8 +177,8 @@ export default compose( withBatchedRendering((props) => { const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); return [ - {items: _.chain(sortedReportActions).last(100).indexBy('reportID').value(), delay: 0}, - {items: _.indexBy(sortedReportActions, 'reportID'), delay: 7000}, + {items: _.chain(sortedReportActions).last(100).indexBy('sequenceNumber').value(), delay: 0}, + {items: _.indexBy(sortedReportActions, 'sequenceNumber'), delay: 7000}, ]; }), )(ReportActionsView); From e0c2063f7e4a3ff7183b8c696828ea9785b38d60 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 17 Sep 2020 10:28:09 -0600 Subject: [PATCH 15/28] Have batch rendering work on normal component updates --- src/components/withBatchedRendering.js | 10 +++++++++- src/page/home/MainView.js | 2 +- src/page/home/report/ReportActionsView.js | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index 0e0f0106998f2..7af770bb49229 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -16,7 +16,7 @@ function getDisplayName(component) { return component.displayName || component.name || 'Component'; } -export default function (batches) { +export default function (propNameToBatch, batches) { return (WrappedComponent) => { class withBatchedRendering extends React.Component { constructor(props) { @@ -37,6 +37,14 @@ export default function (batches) { }); } + componentDidUpdate(prevProps) { + if (_.size(prevProps[propNameToBatch]) !== _.size(this.props[propNameToBatch])) { + this.setState({ + itemsToRender: this.props[propNameToBatch], + }); + } + } + render() { // Spreading props and state is necessary in an HOC where the data cannot be predicted return ( diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 1f4d612c7de72..1d78aa1dc735a 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -103,7 +103,7 @@ export default compose( // The rendering of report views is done in batches. // The first batch are all the reports that are visible in the LHN. // The second batch are all the reports. - withBatchedRendering((props) => { + withBatchedRendering('reports', (props) => { const reportIDInURL = parseInt(props.match.params.reportID, 10); const isReportVisible = report => report.isUnread || report.pinnedReport || report.reportID === reportIDInURL; return [ diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index b904bcc131bc8..9ca302a33034f 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -174,11 +174,11 @@ export default compose( // The rendering of report actions happens in batches. // The first batch of actions is limited to the 100 most recent actions. // The second batch is all of the rest of the actions. - withBatchedRendering((props) => { + withBatchedRendering('reportActions', (props) => { const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); return [ {items: _.chain(sortedReportActions).last(100).indexBy('sequenceNumber').value(), delay: 0}, - {items: _.indexBy(sortedReportActions, 'sequenceNumber'), delay: 7000}, + {items: _.indexBy(sortedReportActions, 'sequenceNumber'), delay: 1000}, ]; }), )(ReportActionsView); From a7388c363a4f37ed91276fdfb93d782710a92a87 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 17 Sep 2020 11:18:44 -0600 Subject: [PATCH 16/28] Prevent batches from overwriting existing props better --- src/components/withBatchedRendering.js | 5 +++-- src/page/home/MainView.js | 26 ++++++++++++++++------- src/page/home/report/ReportActionsView.js | 23 ++++++++++++++------ 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index 7af770bb49229..f060573f1e215 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -28,10 +28,10 @@ export default function (propNameToBatch, batches) { } componentDidMount() { - _.each(batches(this.props), (batch) => { + _.each(batches, (batch) => { setTimeout(() => { this.setState({ - itemsToRender: batch.items, + itemsToRender: batch.items(this.props), }); }, batch.delay || 0); }); @@ -39,6 +39,7 @@ export default function (propNameToBatch, batches) { componentDidUpdate(prevProps) { if (_.size(prevProps[propNameToBatch]) !== _.size(this.props[propNameToBatch])) { + // eslint-disable-next-line react/no-did-update-set-state this.setState({ itemsToRender: this.props[propNameToBatch], }); diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 1d78aa1dc735a..16496f0fe0f41 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -103,12 +103,22 @@ export default compose( // The rendering of report views is done in batches. // The first batch are all the reports that are visible in the LHN. // The second batch are all the reports. - withBatchedRendering('reports', (props) => { - const reportIDInURL = parseInt(props.match.params.reportID, 10); - const isReportVisible = report => report.isUnread || report.pinnedReport || report.reportID === reportIDInURL; - return [ - {items: _.pick(props.reports, isReportVisible), delay: 0}, - {items: props.reports, delay: 5000}, - ]; - }), + withBatchedRendering('reports', [ + { + items: (props) => { + const reportIDInURL = parseInt(props.match.params.reportID, 10); + const isReportVisible = report => ( + report.isUnread + || report.pinnedReport + || report.reportID === reportIDInURL + ); + return _.pick(props.reports, isReportVisible); + }, + delay: 0, + }, + { + items: props => props.reports, + delay: 5000, + }, + ]), )(MainView); diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 9ca302a33034f..75a92a07f1ec6 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -174,11 +174,20 @@ export default compose( // The rendering of report actions happens in batches. // The first batch of actions is limited to the 100 most recent actions. // The second batch is all of the rest of the actions. - withBatchedRendering('reportActions', (props) => { - const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); - return [ - {items: _.chain(sortedReportActions).last(100).indexBy('sequenceNumber').value(), delay: 0}, - {items: _.indexBy(sortedReportActions, 'sequenceNumber'), delay: 1000}, - ]; - }), + withBatchedRendering('reportActions', [ + { + items: (props) => { + const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); + return _.chain(sortedReportActions).last(100).indexBy('sequenceNumber').value(); + }, + delay: 0, + }, + { + items: (props) => { + const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber'); + return _.indexBy(sortedReportActions, 'sequenceNumber'); + }, + delay: 7000, + }, + ]), )(ReportActionsView); From 8d6f0cf5b8184d1ee4b2465ccb782369611b3bf7 Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Thu, 17 Sep 2020 11:26:30 -0600 Subject: [PATCH 17/28] Moar comments --- src/page/home/MainView.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 16496f0fe0f41..ab87a3f7a24f0 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -39,6 +39,9 @@ const defaultProps = { // There are only two pieces of data this component cares about: // - reportID comes from the collection and is used to render ReportView, this data will never change after first render // - match.params.reportID comes from React Router and will change anytime a report is selected from LHN +// Using PureComponent will ensure this component only re-renders if a new report is added to the collection. +// PureComponent shallowly compares props. This means changes to individual reports will not cause a re-render, +// but the addition or removal of a report will cause a render. class MainView extends React.PureComponent { /** * Looks to see if the reportID matches the report ID in the URL because that From 971442e7b923e3c17cea623041370f10c6d87efb Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 21 Sep 2020 14:34:58 -0600 Subject: [PATCH 18/28] Remove extra prop --- src/components/withBatchedRendering.js | 10 +++++----- src/page/home/MainView.js | 13 +++---------- src/page/home/report/ReportActionsView.js | 22 ++++------------------ 3 files changed, 12 insertions(+), 33 deletions(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index f060573f1e215..250c0b0993016 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -22,16 +22,14 @@ export default function (propNameToBatch, batches) { constructor(props) { super(props); - this.state = { - itemsToRender: null, - }; + this.state = {}; } componentDidMount() { _.each(batches, (batch) => { setTimeout(() => { this.setState({ - itemsToRender: batch.items(this.props), + [propNameToBatch]: batch.items(this.props), }); }, batch.delay || 0); }); @@ -47,11 +45,13 @@ export default function (propNameToBatch, batches) { } render() { + const propsToPass = _.omit(this.props, propNameToBatch); + // Spreading props and state is necessary in an HOC where the data cannot be predicted return ( diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index ab87a3f7a24f0..788a3abfdfd4a 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -20,17 +20,10 @@ const propTypes = { reports: PropTypes.objectOf(PropTypes.shape({ reportID: PropTypes.number, })), - - /* From withBatchedRendering() */ - // The specific items that need to be rendered - itemsToRender: PropTypes.PropTypes.objectOf(PropTypes.shape({ - reportID: PropTypes.number, - })), }; const defaultProps = { reports: {}, - itemsToRender: {}, }; // Since this component is connected to an Ion collection, it's props are updated anytime a single report @@ -57,13 +50,13 @@ class MainView extends React.PureComponent { } render() { - if (!_.size(this.props.itemsToRender)) { + if (!_.size(this.props.reports)) { return null; } // The styles for each of our report views. Basically, they are all hidden except for the one matching the // reportID in the URL - const reportStyles = _.reduce(this.props.itemsToRender, (memo, report) => { + const reportStyles = _.reduce(this.props.reports, (memo, report) => { const finalData = {...memo}; let reportStyle; @@ -79,7 +72,7 @@ class MainView extends React.PureComponent { return ( <> - {_.map(this.props.itemsToRender, report => report.reportName && ( + {_.map(this.props.reports, report => report.reportName && ( - {_.map(this.props.itemsToRender, (item, index) => ( + {_.map(this.props.reportActions, (item, index) => ( Date: Mon, 21 Sep 2020 14:35:12 -0600 Subject: [PATCH 19/28] Remove componentDidUpdate --- src/page/home/report/ReportActionsView.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 60e10c4b20afb..2eb17489516ea 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -43,10 +43,6 @@ class ReportActionsView extends React.Component { fetchActions(this.props.reportID); } - componentDidUpdate(prevProps) { - const isReportVisible = this.props.reportID === parseInt(this.props.match.params.reportID, 10); - } - componentWillUnmount() { this.keyboardEvent.remove(); } From b189b8c7828494517eed4db4b1f002c25b7c888c Mon Sep 17 00:00:00 2001 From: Tim Golen Date: Mon, 21 Sep 2020 14:38:37 -0600 Subject: [PATCH 20/28] Put back componentDidUpdate --- src/page/home/report/ReportActionsView.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 2eb17489516ea..fcb350d22bb76 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -43,6 +43,16 @@ class ReportActionsView extends React.Component { fetchActions(this.props.reportID); } + componentDidUpdate(prevProps) { + const isReportVisible = this.props.reportID === parseInt(this.props.match.params.reportID, 10); + + // When the number of actions change, wait three seconds, then record the max action + // This will make the unread indicator go away if you receive comments in the same chat you're looking at + if (isReportVisible && _.size(prevProps.reportActions) !== _.size(this.props.reportActions)) { + setTimeout(this.recordMaxAction, 3000); + } + } + componentWillUnmount() { this.keyboardEvent.remove(); } From f344c2bb0fe1e31f10e46d3545de80b9149a7de3 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 14 Oct 2020 19:25:36 -0700 Subject: [PATCH 21/28] fix conflicts --- src/components/withBatchedRendering.js | 32 ++++++++++++++++------- src/page/home/MainView.js | 4 ++- src/page/home/report/ReportActionsView.js | 1 - src/page/home/report/ReportView.js | 4 --- src/page/home/sidebar/ChatSwitcherView.js | 2 +- 5 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index 250c0b0993016..483be3a7c4e23 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -21,29 +21,41 @@ export default function (propNameToBatch, batches) { class withBatchedRendering extends React.Component { constructor(props) { super(props); - + this.timers = []; this.state = {}; } componentDidMount() { _.each(batches, (batch) => { - setTimeout(() => { - this.setState({ - [propNameToBatch]: batch.items(this.props), - }); - }, batch.delay || 0); + this.timers.push( + setTimeout(() => { + this.setItemsToRender(batch.items(this.props)); + }, batch.delay || 0) + ); }); } componentDidUpdate(prevProps) { if (_.size(prevProps[propNameToBatch]) !== _.size(this.props[propNameToBatch])) { - // eslint-disable-next-line react/no-did-update-set-state - this.setState({ - itemsToRender: this.props[propNameToBatch], - }); + this.setItemsToRender(this.props[propNameToBatch]); } } + componentWillUnmount() { + _.each(this.timers, timerID => clearTimeout(timerID)); + } + + /** + * Sets items to the state key that matches the defined propNameToBatch + * + * @param {*} items + */ + setItemsToRender(items) { + this.setState({ + [propNameToBatch]: items, + }); + } + render() { const propsToPass = _.omit(this.props, propNameToBatch); diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 7a9bd82c79dea..37d97f6f3afb0 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -46,12 +46,14 @@ class MainView extends Component { // The styles for each of our reports. Basically, they are all hidden except for the one matching the // reportID in the URL + let activeReportID; const reportStyles = _.reduce(this.props.reports, (memo, report) => { const isActiveReport = reportIDInUrl === report.reportID; const finalData = {...memo}; let reportStyle; - if (this.isReportIDMatchingURL(report.reportID)) { + if (isActiveReport) { + activeReportID = report.reportID; reportStyle = [styles.dFlex, styles.flex1]; } else { reportStyle = [styles.dNone]; diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 1d937e7751699..42b695caf2f81 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -217,7 +217,6 @@ ReportActionsView.propTypes = propTypes; ReportActionsView.defaultProps = defaultProps; export default compose( - withRouter, withIon({ reportActions: { key: ({reportID}) => `${IONKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, diff --git a/src/page/home/report/ReportView.js b/src/page/home/report/ReportView.js index d246225ef3a37..4032f706c71de 100644 --- a/src/page/home/report/ReportView.js +++ b/src/page/home/report/ReportView.js @@ -13,10 +13,6 @@ const propTypes = { // Whether or not this report is the one that is currently being viewed isActiveReport: PropTypes.bool.isRequired, - - // These are styles to apply to the report view - // eslint-disable-next-line react/forbid-prop-types - style: PropTypes.any.isRequired, }; // This is a PureComponent so that it only re-renders when the reportID changes or when the report changes from diff --git a/src/page/home/sidebar/ChatSwitcherView.js b/src/page/home/sidebar/ChatSwitcherView.js index 4630fd2f97d9e..f9a849871eec2 100644 --- a/src/page/home/sidebar/ChatSwitcherView.js +++ b/src/page/home/sidebar/ChatSwitcherView.js @@ -296,7 +296,7 @@ class ChatSwitcherView extends React.Component { } }} onChangeText={this.updateSearch} - onClearButtonClick={this.reset} + onClearButtonClick={() => this.reset()} onFocus={this.triggerOnFocusCallback} onKeyPress={this.handleKeyPress} /> From 34a0fa671299f779634e831c6df722f8203e8f36 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 14 Oct 2020 19:35:38 -0700 Subject: [PATCH 22/28] clean up and add some comments --- src/Expensify.js | 2 +- src/components/withBatchedRendering.js | 23 +++++++++++++++++------ src/components/withIon.js | 4 ++-- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/Expensify.js b/src/Expensify.js index f2186d970d6f4..bf627e6901af4 100644 --- a/src/Expensify.js +++ b/src/Expensify.js @@ -22,7 +22,7 @@ import ROUTES from './ROUTES'; Ion.init(); const propTypes = { - /* From withIon() */ + /* Ion Props */ // A route set by Ion that we will redirect to if present. Always empty on app init. redirectTo: PropTypes.string, diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index 483be3a7c4e23..c0ee0983d85d4 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -3,7 +3,7 @@ * Example: in MainView.js only the visible reports should be rendered in the first batch, * then all other reports are rendered in the second batch */ -import React from 'react'; +import React, {Component} from 'react'; import _ from 'underscore'; /** @@ -18,7 +18,7 @@ function getDisplayName(component) { export default function (propNameToBatch, batches) { return (WrappedComponent) => { - class withBatchedRendering extends React.Component { + class withBatchedRendering extends Component { constructor(props) { super(props); this.timers = []; @@ -36,19 +36,23 @@ export default function (propNameToBatch, batches) { } componentDidUpdate(prevProps) { + // We need this to allow the flow of props down from a parent component + // to work normally after all the batches have finished rendering if (_.size(prevProps[propNameToBatch]) !== _.size(this.props[propNameToBatch])) { this.setItemsToRender(this.props[propNameToBatch]); } } componentWillUnmount() { - _.each(this.timers, timerID => clearTimeout(timerID)); + // We need to clean up any timers when the component unmounts or else + // we'll call set state on an unmounting component. + this.cancelBatchTimers() } /** * Sets items to the state key that matches the defined propNameToBatch * - * @param {*} items + * @param {Object|Array} items - typically a collection of some kind */ setItemsToRender(items) { this.setState({ @@ -56,10 +60,17 @@ export default function (propNameToBatch, batches) { }); } + /** + * Cancels all the timers + */ + cancelBatchTimers() { + _.each(this.timers, timerID => clearTimeout(timerID)); + } + render() { + // We must remove the original prop that we are splitting into chunks + // since we only want our processed versions to be passed as a prop. const propsToPass = _.omit(this.props, propNameToBatch); - - // Spreading props and state is necessary in an HOC where the data cannot be predicted return ( Date: Wed, 14 Oct 2020 19:37:19 -0700 Subject: [PATCH 23/28] remove some changes to clear up diff a bit --- src/page/SignInPage.js | 2 +- src/page/home/HeaderView.js | 2 +- src/page/home/sidebar/ChatSwitcherView.js | 2 +- src/page/home/sidebar/SidebarBottom.js | 2 +- src/page/home/sidebar/SidebarLinks.js | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/page/SignInPage.js b/src/page/SignInPage.js index 59517b76540e9..b696d3ac96640 100644 --- a/src/page/SignInPage.js +++ b/src/page/SignInPage.js @@ -25,7 +25,7 @@ const propTypes = { // eslint-disable-next-line react/forbid-prop-types match: PropTypes.object.isRequired, - /* From withIon() */ + /* Ion Props */ // The session of the logged in person session: PropTypes.shape({ diff --git a/src/page/home/HeaderView.js b/src/page/home/HeaderView.js index 5fbab86c134f9..863e8b3e92b17 100644 --- a/src/page/home/HeaderView.js +++ b/src/page/home/HeaderView.js @@ -16,7 +16,7 @@ const propTypes = { // Decides whether we should show the hamburger menu button shouldShowHamburgerButton: PropTypes.bool.isRequired, - /* From withIon() */ + /* Ion Props */ // The report currently being looked at report: PropTypes.shape({ // Name of the report diff --git a/src/page/home/sidebar/ChatSwitcherView.js b/src/page/home/sidebar/ChatSwitcherView.js index f9a849871eec2..1135e299f990e 100644 --- a/src/page/home/sidebar/ChatSwitcherView.js +++ b/src/page/home/sidebar/ChatSwitcherView.js @@ -30,7 +30,7 @@ const propTypes = { // A method that is triggered when the TextInput loses focus onBlur: PropTypes.func.isRequired, - /* From withIon() */ + /* Ion Props */ // All of the personal details for everyone // The keys of this object are the logins of the users, and the values are an object diff --git a/src/page/home/sidebar/SidebarBottom.js b/src/page/home/sidebar/SidebarBottom.js index 2b51782e3687c..e53f8beb790ba 100644 --- a/src/page/home/sidebar/SidebarBottom.js +++ b/src/page/home/sidebar/SidebarBottom.js @@ -14,7 +14,7 @@ const propTypes = { // Safe area insets required for mobile devices margins insets: SafeAreaInsetPropTypes.isRequired, - /* From withIon() */ + /* Ion Props */ // The personal details of the person who is logged in myPersonalDetails: PropTypes.shape({ diff --git a/src/page/home/sidebar/SidebarLinks.js b/src/page/home/sidebar/SidebarLinks.js index a27a22bb9a382..6af6c4e519053 100644 --- a/src/page/home/sidebar/SidebarLinks.js +++ b/src/page/home/sidebar/SidebarLinks.js @@ -30,7 +30,7 @@ const propTypes = { // Safe area insets required for mobile devices margins insets: SafeAreaInsetPropTypes.isRequired, - /* From withIon() */ + /* Ion Props */ // List of reports reports: PropTypes.objectOf(PropTypes.shape({ From c4e22d4714f08a3c5c63cb7c7239f42691e145c0 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 14 Oct 2020 19:39:33 -0700 Subject: [PATCH 24/28] fix another diff --- src/components/withIon.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/withIon.js b/src/components/withIon.js index 41fc5c619d538..0fc59fac944f2 100644 --- a/src/components/withIon.js +++ b/src/components/withIon.js @@ -10,8 +10,8 @@ import Ion from '../lib/Ion'; /** * Returns the display name of a component * - * @param {Object} component - * @returns {String} + * @param {object} component + * @returns {string} */ function getDisplayName(component) { return component.displayName || component.name || 'Component'; From 98066982a2924703c39fcac4fc229c7e3b07ecac Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 14 Oct 2020 19:45:15 -0700 Subject: [PATCH 25/28] more clean up --- src/page/home/MainView.js | 8 ++++---- src/page/home/report/ReportActionsView.js | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/page/home/MainView.js b/src/page/home/MainView.js index 37d97f6f3afb0..b826c5a44446d 100644 --- a/src/page/home/MainView.js +++ b/src/page/home/MainView.js @@ -3,11 +3,11 @@ import {View} from 'react-native'; import PropTypes from 'prop-types'; import _ from 'underscore'; import ReportView from './report/ReportView'; +import withIon from '../../components/withIon'; import IONKEYS from '../../IONKEYS'; import styles from '../../style/StyleSheet'; import {withRouter} from '../../lib/Router'; import compose from '../../lib/compose'; -import withIon from '../../components/withIon'; import withBatchedRendering from '../../components/withBatchedRendering'; const propTypes = { @@ -15,9 +15,9 @@ const propTypes = { // eslint-disable-next-line react/forbid-prop-types match: PropTypes.object.isRequired, - /* From withIon() */ - // The reports from Ion that exist and will have a view rendered for them - // eslint-disable-next-line react/no-unused-prop-types + /* Ion Props */ + + // List of reports to display reports: PropTypes.objectOf(PropTypes.shape({ reportID: PropTypes.number, })), diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 42b695caf2f81..2b68d07277dea 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -134,7 +134,8 @@ class ReportActionsView extends React.Component { * action when scrolled */ recordMaxAction() { - const maxVisibleSequenceNumber = _.chain(this.props.reportActions) + const reportActions = lodashGet(this.props, 'reportActions', {}); + const maxVisibleSequenceNumber = _.chain(reportActions) .pluck('sequenceNumber') .max() .value(); From 0559a57ed74afca623a2f7b4588d7adf84e23244 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 14 Oct 2020 19:47:40 -0700 Subject: [PATCH 26/28] fix style --- src/components/withBatchedRendering.js | 2 +- src/page/home/report/ReportActionsView.js | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index c0ee0983d85d4..0cb524d9b6c36 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -46,7 +46,7 @@ export default function (propNameToBatch, batches) { componentWillUnmount() { // We need to clean up any timers when the component unmounts or else // we'll call set state on an unmounting component. - this.cancelBatchTimers() + this.cancelBatchTimers(); } /** diff --git a/src/page/home/report/ReportActionsView.js b/src/page/home/report/ReportActionsView.js index 2b68d07277dea..c97727c76fc67 100644 --- a/src/page/home/report/ReportActionsView.js +++ b/src/page/home/report/ReportActionsView.js @@ -2,6 +2,7 @@ import React from 'react'; import {View, Keyboard} from 'react-native'; import PropTypes from 'prop-types'; import _ from 'underscore'; +import lodashGet from 'lodash.get'; import Text from '../../../components/Text'; import withIon from '../../../components/withIon'; import {fetchActions, updateLastReadActionID} from '../../../lib/actions/Report'; From 1230443b8091bbbe95341e69fdd6a0b2b751e194 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 15 Oct 2020 11:37:04 -0700 Subject: [PATCH 27/28] fix bad merge --- src/pages/home/report/ReportActionsView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 993a428d5daf7..81cc4d29d45ba 100644 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -10,7 +10,7 @@ import IONKEYS from '../../../IONKEYS'; import ReportActionItem from './ReportActionItem'; import styles from '../../../styles/StyleSheet'; import ReportActionPropTypes from './ReportActionPropTypes'; -import compose from '../../../lib/compose'; +import compose from '../../../libs/compose'; import withBatchedRendering from '../../../components/withBatchedRendering'; import InvertedFlatList from '../../../components/InvertedFlatList'; import {lastItem} from '../../../libs/CollectionUtils'; From 5faccd6b09bf85f31e2489a9daa3bf087673bfbe Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 15 Oct 2020 13:55:08 -0700 Subject: [PATCH 28/28] just use single line instead of method; ; --- src/components/withBatchedRendering.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/components/withBatchedRendering.js b/src/components/withBatchedRendering.js index 0cb524d9b6c36..97b81ed05b2b9 100644 --- a/src/components/withBatchedRendering.js +++ b/src/components/withBatchedRendering.js @@ -46,7 +46,7 @@ export default function (propNameToBatch, batches) { componentWillUnmount() { // We need to clean up any timers when the component unmounts or else // we'll call set state on an unmounting component. - this.cancelBatchTimers(); + _.each(this.timers, timerID => clearTimeout(timerID)); } /** @@ -60,13 +60,6 @@ export default function (propNameToBatch, batches) { }); } - /** - * Cancels all the timers - */ - cancelBatchTimers() { - _.each(this.timers, timerID => clearTimeout(timerID)); - } - render() { // We must remove the original prop that we are splitting into chunks // since we only want our processed versions to be passed as a prop.