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

Cleanup props and callbacks for OptionsList #13319

Merged
merged 22 commits into from
Dec 7, 2022
Merged

Cleanup props and callbacks for OptionsList #13319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
2c75363
Simplify logic
tgolen Dec 4, 2022
fc42750
Remove useless code
tgolen Dec 4, 2022
2ad77db
Move callback to a class method
tgolen Dec 4, 2022
8007495
Move callback to a class method
tgolen Dec 4, 2022
581678a
Move callback to a class method
tgolen Dec 4, 2022
1e9c971
Move callback to a class method
tgolen Dec 4, 2022
4fc472a
Move callback to a class method
tgolen Dec 4, 2022
fa49120
Cleanup memo logic
tgolen Dec 4, 2022
354c80b
Remove unused props
tgolen Dec 4, 2022
0be8376
Remove unused options
tgolen Dec 4, 2022
7293649
Cleanup indentation
tgolen Dec 4, 2022
1de348b
Cleanup logic
tgolen Dec 4, 2022
c5e14d1
Remove unnecessary prop
tgolen Dec 4, 2022
ac2e2e7
Remove unnecessary prop
tgolen Dec 4, 2022
e52275e
Rename prop
tgolen Dec 4, 2022
ae077b2
Fix indentation
tgolen Dec 5, 2022
47b19ef
Remove unused import
tgolen Dec 5, 2022
ce485c4
Merge branch 'main' into tgolen-optimize-optionlist
tgolen Dec 6, 2022
d1458c7
Add early return instead
tgolen Dec 6, 2022
9d867b9
Improve comment for prop
tgolen Dec 6, 2022
afc514c
Fix memo function
tgolen Dec 6, 2022
0b40699
Remove commented out code
tgolen Dec 7, 2022
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
7 changes: 2 additions & 5 deletions src/components/IOUConfirmationList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ const propTypes = {
searchText: PropTypes.string,
text: PropTypes.string,
keyForList: PropTypes.string,
isPinned: PropTypes.bool,
isUnread: PropTypes.bool,
reportID: PropTypes.string,
// eslint-disable-next-line react/forbid-prop-types
participantsList: PropTypes.arrayOf(PropTypes.object),
Expand Down Expand Up @@ -242,7 +240,7 @@ class IOUConfirmationList extends Component {
this.setState((prevState) => {
const newParticipants = _.map(prevState.participants, (participant) => {
if (participant.login === option.login) {
return {...option, selected: !option.selected};
return {...participant, selected: !participant.selected};
}
return participant;
});
Expand Down Expand Up @@ -291,8 +289,7 @@ class IOUConfirmationList extends Component {
canSelectMultipleOptions={canModifyParticipants}
disableArrowKeysActions={!canModifyParticipants}
isDisabled={!canModifyParticipants}
hideAdditionalOptionStates
forceTextUnreadStyle
boldStyle
autoFocus
shouldDelayFocus
shouldTextInputAppearBelowOptions
Expand Down
30 changes: 14 additions & 16 deletions src/components/LHNOptionsList/OptionRowLHN.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const OptionRowLHN = (props) => {
? styles.sidebarLinkActiveText
: styles.sidebarLinkText;
const textUnreadStyle = optionItem.isUnread
? [textStyle, styles.sidebarLinkTextUnread] : [textStyle];
? [textStyle, styles.sidebarLinkTextBold] : [textStyle];
const displayNameStyle = StyleUtils.combineStyles([styles.optionDisplayName, styles.optionDisplayNameCompact, ...textUnreadStyle], props.style);
const textPillStyle = props.isFocused
? [styles.ml1, StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.icon, 0.5)]
Expand Down Expand Up @@ -215,23 +215,21 @@ const OptionRowLHN = (props) => {
accessible={false}
>
{optionItem.hasDraftComment && (
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.draftedMessage')}
>
<Icon src={Expensicons.Pencil} height={16} width={16} />
</View>
)}
{optionItem.hasOutstandingIOU && (
<IOUBadge iouReportID={optionItem.iouReportID} />
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.draftedMessage')}
>
<Icon src={Expensicons.Pencil} height={16} width={16} />
</View>
)}
{optionItem.hasOutstandingIOU && <IOUBadge iouReportID={optionItem.iouReportID} />}
{optionItem.isPinned && (
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.chatPinned')}
>
<Icon src={Expensicons.Pin} height={16} width={16} />
</View>
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.chatPinned')}
>
<Icon src={Expensicons.Pin} height={16} width={16} />
</View>
)}
</View>
</TouchableOpacity>
Expand Down
150 changes: 26 additions & 124 deletions src/components/OptionRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import * as Expensicons from './Icon/Expensicons';
import MultipleAvatars from './MultipleAvatars';
import Hoverable from './Hoverable';
import DisplayNames from './DisplayNames';
import IOUBadge from './IOUBadge';
import themeColors from '../styles/themes/default';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import Text from './Text';
Expand All @@ -27,15 +26,6 @@ import * as ReportUtils from '../libs/ReportUtils';
import variables from '../styles/variables';

const propTypes = {
/** The accessibility hint for the entire option row. Primarily used for unit testing to identify the component */
accessibilityHint: PropTypes.string,

/** The accessibility hint for alternative text label. Primarily used for unit testing to identify the component */
alternateTextAccessibilityLabel: PropTypes.string,

/** Background Color of the Option Row */
backgroundColor: PropTypes.string,

/** Style for hovered state */
// eslint-disable-next-line react/forbid-prop-types
hoverStyle: PropTypes.object,
Expand All @@ -49,17 +39,14 @@ const propTypes = {
/** A function that is called when an option is selected. Selected option is passed as a param */
onSelectRow: PropTypes.func,

/** A flag to indicate whether to show additional optional states, such as pin and draft icons */
hideAdditionalOptionStates: PropTypes.bool,

/** Whether we should show the selected state */
showSelectedState: PropTypes.bool,

/** Whether this item is selected */
isSelected: PropTypes.bool,

/** Force the text style to be the unread style */
forceTextUnreadStyle: PropTypes.bool,
/** Display the text of the option in bold font style */
boldStyle: PropTypes.bool,

/** Whether to show the title tooltip */
showTitleTooltip: PropTypes.bool,
Expand All @@ -79,14 +66,10 @@ const propTypes = {
};

const defaultProps = {
accessibilityHint: '',
alternateTextAccessibilityLabel: '',
backgroundColor: themeColors.appBG,
hoverStyle: styles.sidebarLinkHover,
hideAdditionalOptionStates: false,
showSelectedState: false,
isSelected: false,
forceTextUnreadStyle: false,
boldStyle: false,
showTitleTooltip: false,
mode: 'default',
onSelectRow: () => {},
Expand All @@ -101,8 +84,8 @@ const OptionRow = (props) => {
const textStyle = props.optionIsFocused
? styles.sidebarLinkActiveText
: styles.sidebarLinkText;
const textUnreadStyle = (props.option.isUnread || props.forceTextUnreadStyle)
? [textStyle, styles.sidebarLinkTextUnread] : [textStyle];
const textUnreadStyle = (props.boldStyle)
? [textStyle, styles.sidebarLinkTextBold] : [textStyle];
const displayNameStyle = StyleUtils.combineStyles(props.mode === CONST.OPTION_MODE.COMPACT
? [styles.optionDisplayName, ...textUnreadStyle, styles.optionDisplayNameCompact, styles.mr2]
: [styles.optionDisplayName, ...textUnreadStyle], props.style);
Expand Down Expand Up @@ -164,14 +147,13 @@ const OptionRow = (props) => {
styles.justifyContentBetween,
styles.sidebarLink,
styles.sidebarLinkInner,
StyleUtils.getBackgroundColorStyle(props.backgroundColor),
props.optionIsFocused ? styles.sidebarLinkActive : null,
hovered && !props.optionIsFocused ? props.hoverStyle : null,
props.isDisabled && styles.cursorDisabled,
props.shouldHaveOptionSeparator && styles.borderTop,
]}
>
<View accessibilityHint={props.accessibilityHint} style={sidebarInnerRowStyle}>
<View style={sidebarInnerRowStyle}>
<View
style={[
styles.flexRow,
Expand All @@ -194,7 +176,6 @@ const OptionRow = (props) => {
icons={props.option.icons}
size={props.mode === CONST.OPTION_MODE.COMPACT ? CONST.AVATAR_SIZE.SMALL : CONST.AVATAR_SIZE.DEFAULT}
secondAvatarStyle={[
StyleUtils.getBackgroundAndBorderStyle(props.backgroundColor),
props.optionIsFocused
? StyleUtils.getBackgroundAndBorderStyle(focusedBackgroundColor)
: undefined,
Expand All @@ -219,7 +200,6 @@ const OptionRow = (props) => {
/>
{props.option.alternateText ? (
<Text
accessibilityLabel={props.alternateTextAccessibilityLabel}
style={alternateTextStyle}
numberOfLines={1}
>
Expand Down Expand Up @@ -247,40 +227,19 @@ const OptionRow = (props) => {
{props.showSelectedState && <SelectCircle isChecked={props.isSelected} />}
</View>
</View>
{!props.hideAdditionalOptionStates && (
{props.option.customIcon && (
<View
style={[styles.flexRow, styles.alignItemsCenter]}
accessible={false}
>
{props.option.hasDraftComment && (
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.draftedMessage')}
>
<Icon src={Expensicons.Pencil} height={16} width={16} />
</View>
)}
{props.option.hasOutstandingIOU && (
<IOUBadge iouReportID={props.option.iouReportID} />
)}
{props.option.isPinned && (
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.chatPinned')}
>
<Icon src={Expensicons.Pin} height={16} width={16} />
</View>
)}
{Boolean(props.option.customIcon) && (
<View>
<Icon
src={lodashGet(props.option, 'customIcon.src', '')}
height={16}
width={16}
fill={lodashGet(props.option, 'customIcon.color')}
/>
</View>
)}
<View>
<Icon
src={lodashGet(props.option, 'customIcon.src', '')}
height={16}
width={16}
fill={lodashGet(props.option, 'customIcon.color')}
/>
</View>
</View>
)}
</TouchableOpacity>
Expand All @@ -295,71 +254,14 @@ OptionRow.defaultProps = defaultProps;
OptionRow.displayName = 'OptionRow';

// It it very important to use React.memo here so SectionList items will not unnecessarily re-render
export default withLocalize(memo(OptionRow, (prevProps, nextProps) => {
if (prevProps.optionIsFocused !== nextProps.optionIsFocused) {
return false;
}

if (prevProps.isSelected !== nextProps.isSelected) {
return false;
}

if (prevProps.mode !== nextProps.mode) {
return false;
}

if (prevProps.option.isUnread !== nextProps.option.isUnread) {
return false;
}

if (prevProps.option.alternateText !== nextProps.option.alternateText) {
return false;
}

if (prevProps.option.descriptiveText !== nextProps.option.descriptiveText) {
return false;
}

if (prevProps.option.hasDraftComment !== nextProps.option.hasDraftComment) {
return false;
}

if (prevProps.option.isPinned !== nextProps.option.isPinned) {
return false;
}

if (prevProps.option.hasOutstandingIOU !== nextProps.option.hasOutstandingIOU) {
return false;
}

if (!_.isEqual(prevProps.option.icons, nextProps.option.icons)) {
return false;
}

// Re-render when the text changes
if (prevProps.option.text !== nextProps.option.text) {
return false;
}

if (prevProps.showSelectedState !== nextProps.showSelectedState) {
return false;
}

if (prevProps.isDisabled !== nextProps.isDisabled) {
return false;
}

if (prevProps.showTitleTooltip !== nextProps.showTitleTooltip) {
return false;
}

if (prevProps.backgroundColor !== nextProps.backgroundColor) {
return false;
}

if (prevProps.option.brickRoadIndicator !== nextProps.option.brickRoadIndicator) {
return false;
}

return true;
}));
export default withLocalize(memo(OptionRow, (prevProps, nextProps) => prevProps.optionIsFocused === nextProps.optionIsFocused
&& prevProps.isSelected === nextProps.isSelected
&& prevProps.mode === nextProps.mode
&& prevProps.option.alternateText === nextProps.option.alternateText
&& prevProps.option.descriptiveText === nextProps.option.descriptiveText
&& _.isEqual(prevProps.option.icons, nextProps.option.icons)
&& prevProps.option.text === nextProps.option.text
&& prevProps.showSelectedState === nextProps.showSelectedState
&& prevProps.isDisabled === nextProps.isDisabled
&& prevProps.showTitleTooltip === nextProps.showTitleTooltip
&& prevProps.option.brickRoadIndicator === nextProps.option.brickRoadIndicator));
34 changes: 9 additions & 25 deletions src/components/OptionsList/BaseOptionsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ const propTypes = {
/** Determines whether the keyboard gets dismissed in response to a drag */
keyboardDismissMode: PropTypes.string,

/** Called when the user begins to drag the scroll view */
/** Called when the user begins to drag the scroll view. Only used for the native component */
onScrollBeginDrag: PropTypes.func,

/** Callback executed on scroll */
/** Callback executed on scroll. Only used for web/desktop component */
onScroll: PropTypes.func,

...optionsListPropTypes,
Expand Down Expand Up @@ -47,23 +47,11 @@ class BaseOptionsList extends Component {
}

shouldComponentUpdate(nextProps) {
if (nextProps.focusedIndex !== this.props.focusedIndex) {
return true;
}

if (nextProps.selectedOptions.length !== this.props.selectedOptions.length) {
return true;
}

if (nextProps.headerMessage !== this.props.headerMessage) {
return true;
}

if (!_.isEqual(nextProps.sections, this.props.sections)) {
return true;
}

return false;
return nextProps.focusedIndex !== this.props.focusedIndex
|| nextProps.selectedOptions.length !== this.props.selectedOptions.length
|| nextProps.headerMessage !== this.props.headerMessage
|| !_.isEqual(nextProps.sections, this.props.sections)
|| !_.isEqual(nextProps.sections, this.props.sections);
}

componentDidUpdate(prevProps) {
Expand Down Expand Up @@ -170,20 +158,16 @@ class BaseOptionsList extends Component {
renderItem({item, index, section}) {
return (
<OptionRow
alternateTextAccessibilityLabel={this.props.optionRowAlternateTextAccessibilityLabel}
accessibilityHint={this.props.optionRowAccessibilityHint}
option={item}
mode={this.props.optionMode}
showTitleTooltip={this.props.showTitleTooltip}
backgroundColor={this.props.optionBackgroundColor}
hoverStyle={this.props.optionHoveredStyle}
optionIsFocused={!this.props.disableFocusOptions
&& this.props.focusedIndex === (index + section.indexOffset)}
&& this.props.focusedIndex === (index + section.indexOffset)}
onSelectRow={this.props.onSelectRow}
isSelected={Boolean(_.find(this.props.selectedOptions, option => option.login === item.login))}
showSelectedState={this.props.canSelectMultipleOptions}
hideAdditionalOptionStates={this.props.hideAdditionalOptionStates}
forceTextUnreadStyle={this.props.forceTextUnreadStyle}
boldStyle={this.props.boldStyle}
isDisabled={this.props.isDisabled || section.isDisabled}
shouldHaveOptionSeparator={index > 0 && this.props.shouldHaveOptionSeparator}
/>
Expand Down
Loading