-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Improve memoization of LHN by handling the isFocused prop better #28615
Changes from 4 commits
ffd2418
39b4fa3
8a0abb6
4bb01bc
f758aeb
fd9446b
86ee246
72ee59a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ import {withReportCommentDrafts} from '../OnyxProvider'; | |
import SidebarUtils from '../../libs/SidebarUtils'; | ||
import compose from '../../libs/compose'; | ||
import ONYXKEYS from '../../ONYXKEYS'; | ||
import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../withCurrentReportID'; | ||
import OptionRowLHN, {propTypes as basePropTypes, defaultProps as baseDefaultProps} from './OptionRowLHN'; | ||
import * as Report from '../../libs/actions/Report'; | ||
import * as UserUtils from '../../libs/UserUtils'; | ||
|
@@ -20,8 +19,8 @@ import CONST from '../../CONST'; | |
import reportActionPropTypes from '../../pages/home/report/reportActionPropTypes'; | ||
|
||
const propTypes = { | ||
/** If true will disable ever setting the OptionRowLHN to focused */ | ||
shouldDisableFocusOptions: PropTypes.bool, | ||
/** Wether row should be focused */ | ||
isFocused: PropTypes.bool, | ||
|
||
/** List of users' personal details */ | ||
personalDetails: PropTypes.objectOf(participantPropTypes), | ||
|
@@ -51,20 +50,17 @@ const propTypes = { | |
/** The ID of the transaction */ | ||
transactionID: PropTypes.string, | ||
}), | ||
|
||
...withCurrentReportIDPropTypes, | ||
...basePropTypes, | ||
}; | ||
|
||
const defaultProps = { | ||
shouldDisableFocusOptions: false, | ||
isFocused: false, | ||
personalDetails: {}, | ||
fullReport: {}, | ||
policy: {}, | ||
parentReportActions: {}, | ||
transaction: {}, | ||
preferredLocale: CONST.LOCALES.DEFAULT, | ||
...withCurrentReportIDDefaultProps, | ||
...baseDefaultProps, | ||
}; | ||
|
||
|
@@ -75,8 +71,7 @@ const defaultProps = { | |
* re-render if the data really changed. | ||
*/ | ||
function OptionRowLHNData({ | ||
shouldDisableFocusOptions, | ||
currentReportID, | ||
isFocused, | ||
fullReport, | ||
reportActions, | ||
personalDetails, | ||
|
@@ -91,7 +86,6 @@ function OptionRowLHNData({ | |
const reportID = propsToForward.reportID; | ||
// We only want to pass a boolean to the memoized component, | ||
// instead of a changing number (so we prevent unnecessary re-renders). | ||
const isFocused = !shouldDisableFocusOptions && currentReportID === reportID; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can move this comment to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done ✅ |
||
|
||
const parentReportAction = parentReportActions[fullReport.parentReportActionID]; | ||
|
||
|
@@ -172,7 +166,6 @@ const personalDetailsSelector = (personalDetails) => | |
*/ | ||
export default React.memo( | ||
compose( | ||
withCurrentReportID, | ||
withReportCommentDrafts({ | ||
propName: 'comment', | ||
transformValue: (drafts, props) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../withCurrentReportID'; | ||
import OptionRowLHNData from './OptionRowLHNData'; | ||
|
||
const propTypes = { | ||
...withCurrentReportIDPropTypes, | ||
shouldDisableFocusOptions: PropTypes.bool, | ||
}; | ||
|
||
const defaultProps = { | ||
...withCurrentReportIDDefaultProps, | ||
shouldDisableFocusOptions: false, | ||
}; | ||
|
||
function OptionRowLHNDataWithFocus({currentReportID, shouldDisableFocusOptions, ...props}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add some comments stating what this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done ✅ |
||
const isFocused = !shouldDisableFocusOptions && currentReportID === props.reportID; | ||
|
||
return ( | ||
<OptionRowLHNData | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
isFocused={isFocused} | ||
/> | ||
); | ||
} | ||
|
||
OptionRowLHNDataWithFocus.defaultProps = defaultProps; | ||
OptionRowLHNDataWithFocus.propTypes = propTypes; | ||
OptionRowLHNDataWithFocus.displayName = 'OptionRowLHNDataWithFocus'; | ||
|
||
export default withCurrentReportID(OptionRowLHNDataWithFocus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅