Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize the left-hand-nav to only render options when they need to be visible #10800

Merged
merged 77 commits into from
Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from 75 commits
Commits
Show all changes
77 commits
Select commit Hold shift + click to select a range
d00f253
Add error boundary to test rendering
tgolen Sep 2, 2022
0f88560
Fix unit tests
tgolen Sep 2, 2022
7f863d4
Use a custom memoizing function
tgolen Sep 2, 2022
4a4e100
Duplicate the option list component
tgolen Sep 2, 2022
67a2cc4
Remove showTitleTooltip and hideSectionHeaders props
tgolen Sep 2, 2022
fd9d0e2
Remove listContainerStyle prop
tgolen Sep 2, 2022
3bc3974
Remove optionHoveredStyle prop
tgolen Sep 2, 2022
30d6839
Remove selectedOptions prop
tgolen Sep 2, 2022
04ade70
Remove canSelectMultipleOptions prop
tgolen Sep 2, 2022
7e6415f
Remove hideAdditionalOptionStates prop
tgolen Sep 2, 2022
e825e1b
Remove forceTextUnreadStyle prop
tgolen Sep 2, 2022
47f01cb
Remove headerMessage prop
tgolen Sep 2, 2022
5ec8823
Remove isDisabled prop
tgolen Sep 2, 2022
2e35baf
Remove default props
tgolen Sep 2, 2022
a6a2a67
Duplicate the option row component
tgolen Sep 2, 2022
f1550a3
Fix new paths
tgolen Sep 2, 2022
02c1b21
Remove default props from native component
tgolen Sep 2, 2022
2de9592
Remove showTitleTooltip prop
tgolen Sep 2, 2022
a39309b
Remove showSelectedState prop
tgolen Sep 2, 2022
62aacd2
Remove hideAdditionalOptionStates prop
tgolen Sep 2, 2022
ef2d28a
Remove forceTextUnreadStyle prop
tgolen Sep 2, 2022
958d581
Remove isDisabled prop
tgolen Sep 2, 2022
023fb06
Switch to using new LHN option list
tgolen Sep 2, 2022
1b6b6da
Clean up styles
tgolen Sep 2, 2022
eee30e5
Fix default innerRef prop
tgolen Sep 2, 2022
0f211a6
Add a skeleton utils class for new LHN options
tgolen Sep 2, 2022
c0131c3
Move create option to new utils and remove non-report logic
tgolen Sep 2, 2022
078e383
Optimize red brick road code
tgolen Sep 2, 2022
c23c625
Correct lint errors
tgolen Sep 2, 2022
bc10dfb
Remove searchValue variable
tgolen Sep 2, 2022
0e2335c
rename the mode prop to vewMode
tgolen Sep 2, 2022
a2e84e8
Remove background color prop
tgolen Sep 2, 2022
f28ba65
Remove option prop
tgolen Sep 2, 2022
edc7475
Switch to using a flatlist
tgolen Sep 2, 2022
e901579
Rename section prop to data
tgolen Sep 2, 2022
405e4f6
WIP for new report ordering
tgolen Sep 2, 2022
894debb
WIP for new report ordering
tgolen Sep 2, 2022
205e8c3
Remove boolean options
tgolen Sep 2, 2022
1b71313
Fix lint errors
tgolen Sep 2, 2022
ac4c9ab
Fix proptypes
tgolen Sep 2, 2022
61815e7
Correct the item passed to option row
tgolen Sep 2, 2022
350204a
Fix the focused index
tgolen Sep 2, 2022
89d2da2
Update src/components/LHNOptionsList/BaseOptionsListLHN.js
tgolen Sep 5, 2022
e041f92
Import variables and CONST and simplify index property
tgolen Sep 5, 2022
635e621
Merge branch 'tgolen-optimize-sidebarordering' into tgolen-lazy-optio…
tgolen Sep 5, 2022
efe754e
Merge branch 'main' into tgolen-lazy-option-rendering
tgolen Sep 13, 2022
1ed435e
Fix method docs
tgolen Sep 13, 2022
3c1b63f
Fix report actions not being passed properly to OptionsListUtils
tgolen Sep 13, 2022
5eddedb
Fix report actions not being passed properly to OptionsListUtils
tgolen Sep 13, 2022
f8d3e4a
WIP to get tests passing
tgolen Sep 13, 2022
409c6f3
Add some TODOs, rename isOptionFocused prop
tgolen Sep 14, 2022
c03c351
Rename file to have a more proper naming convention
tgolen Sep 14, 2022
6718910
Refactor LHN OptionList to remove a bunch of unneeded props
tgolen Sep 14, 2022
dd5b116
Remove should component update
tgolen Sep 14, 2022
2a40243
Merge branch 'main' into tgolen-lazy-option-rendering
tgolen Sep 16, 2022
cddadd3
Use onLayout since it's only called on mount
tgolen Sep 16, 2022
3af87d7
Remove viewabilityConfig
tgolen Sep 16, 2022
46f0251
Ignore console debug timing logs
tgolen Sep 16, 2022
4b1c407
Fix display names on reports
tgolen Sep 16, 2022
8a9806a
Remove unnecessary logic
tgolen Sep 16, 2022
87fcd06
Unmount components before clearing onyx
tgolen Sep 16, 2022
e6eb624
Uncomment test for draft reports and get it passing
tgolen Sep 16, 2022
7493711
Remove test that doesn't make sense
tgolen Sep 16, 2022
709210b
Ignore a lint error
tgolen Sep 16, 2022
48f0008
Merge branch 'main' into tgolen-lazy-option-rendering
tgolen Sep 19, 2022
ef187ee
Add new proptypes and missing proptype definitions
tgolen Sep 19, 2022
16474e0
Fix typo in comment
tgolen Sep 19, 2022
8240970
Protect against an undefined event
tgolen Sep 19, 2022
546f914
Fix unread style props for the new sidebar
tgolen Sep 19, 2022
8b1a91b
Fix inconsistent module name
tgolen Sep 19, 2022
a191891
Remove unused package
tgolen Sep 19, 2022
2b0adba
Remove unnecessary logic
tgolen Sep 19, 2022
157cf62
Subscribe to some additional keys to ensure data doesn't get stale
tgolen Sep 19, 2022
66bdd9c
Add comments to explain how the onyx subscriptions work
tgolen Sep 19, 2022
dd4d139
Remove all component unmounting
tgolen Sep 19, 2022
23bd4dd
Add workspace exception
tgolen Sep 20, 2022
f6fb287
Remove extra blank line
tgolen Sep 20, 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
13 changes: 13 additions & 0 deletions jest/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,19 @@ jest.mock('../src/libs/BootSplash', () => ({
getVisibilityStatus: jest.fn().mockResolvedValue('hidden'),
}));

jest.mock('react-native-blob-util', () => ({}));

// Turn off the console logs for timing events. They are not relevant for unit tests and create a lot of noise
jest.spyOn(console, 'debug').mockImplementation((...params) => {
if (params[0].indexOf('Timing:') === 0) {
return;
}

// Send the message to console.log but don't re-used console.debug or else this mock method is called in an infinite loop. Instead, just prefix the output with the word "DEBUG"
// eslint-disable-next-line no-console
console.log('DEBUG', ...params);
});

// Local notifications (a.k.a. browser notifications) do not run in native code. Our jest tests will also run against
// any index.native.js files as they are using a react-native plugin. However, it is useful to mock this behavior so that we
// can test the expected web behavior and see if a browser notification would be shown or not.
Expand Down
11 changes: 0 additions & 11 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@
"htmlparser2": "^7.2.0",
"localforage": "^1.10.0",
"lodash": "4.17.21",
"memoize-one": "^6.0.0",
"metro-config": "^0.71.3",
"moment": "^2.29.4",
"moment-timezone": "^0.5.31",
Expand Down
104 changes: 104 additions & 0 deletions src/components/LHNOptionsList/LHNOptionsList.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import _ from 'underscore';
import React, {Component} from 'react';
import {View, FlatList} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../styles/styles';
import OptionRowLHN from './OptionRowLHN';
import variables from '../../styles/variables';
import CONST from '../../CONST';

const propTypes = {
/** Extra styles for the section list container */
// eslint-disable-next-line react/forbid-prop-types
contentContainerStyles: PropTypes.arrayOf(PropTypes.object).isRequired,

/** Sections for the section list */
data: PropTypes.arrayOf(PropTypes.string).isRequired,

/** Index for option to focus on */
focusedIndex: PropTypes.number.isRequired,

/** Callback to fire when a row is selected */
onSelectRow: PropTypes.func.isRequired,

/** Toggle between compact and default view of the option */
optionMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)).isRequired,

/** Callback to execute when the SectionList lays out */
onLayout: PropTypes.func.isRequired,
};

class LHNOptionsList extends Component {
constructor(props) {
super(props);

this.renderItem = this.renderItem.bind(this);
this.getItemLayout = this.getItemLayout.bind(this);
}

/**
* This function is used to compute the layout of any given item in our list. Since we know that each item will have the exact same height, this is a performance optimization
* so that the heights can be determined before the options are rendered. Otherwise, the heights are determined when each option is rendering and it causes a lot of overhead on large
* lists.
*
* @param {Array} data - This is the same as the data we pass into the component
* @param {Number} index the current item's index in the set of data
*
* @returns {Object}
*/
getItemLayout(data, index) {
const optionHeight = this.props.optionMode === CONST.OPTION_MODE.COMPACT ? variables.optionRowHeightCompact : variables.optionRowHeight;
return {
length: optionHeight,
offset: index * optionHeight,
index,
};
}

/**
* Function which renders a row in the list
*
* @param {Object} params
* @param {Object} params.item
* @param {Number} params.index
*
* @return {Component}
*/
renderItem({item, index}) {
return (
<OptionRowLHN
reportID={item}
viewMode={this.props.optionMode}
isFocused={this.props.focusedIndex === index}
onSelectRow={this.props.onSelectRow}
/>
);
}

render() {
return (
<View style={[styles.flex1]}>
<FlatList
indicatorStyle="white"
keyboardShouldPersistTaps="always"
contentContainerStyle={this.props.contentContainerStyles}
showsVerticalScrollIndicator={false}
data={this.props.data}
keyExtractor={item => item}
stickySectionHeadersEnabled={false}
renderItem={this.renderItem}
getItemLayout={this.getItemLayout}
extraData={this.props.focusedIndex}
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
initialNumToRender={5}
maxToRenderPerBatch={5}
windowSize={5}
onLayout={this.props.onLayout}
/>
</View>
);
}
}

LHNOptionsList.propTypes = propTypes;

export default LHNOptionsList;
226 changes: 226 additions & 0 deletions src/components/LHNOptionsList/OptionRowLHN.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,226 @@
import _ from 'underscore';
import lodashGet from 'lodash/get';
import React from 'react';
import PropTypes from 'prop-types';
import {
TouchableOpacity,
View,
StyleSheet,
} from 'react-native';
import styles from '../../styles/styles';
import * as StyleUtils from '../../styles/StyleUtils';
import Icon from '../Icon';
import * as Expensicons from '../Icon/Expensicons';
import MultipleAvatars from '../MultipleAvatars';
import Hoverable from '../Hoverable';
import DisplayNames from '../DisplayNames';
import IOUBadge from '../IOUBadge';
import colors from '../../styles/colors';
import withLocalize, {withLocalizePropTypes} from '../withLocalize';
import Text from '../Text';
import SubscriptAvatar from '../SubscriptAvatar';
import CONST from '../../CONST';
import * as ReportUtils from '../../libs/ReportUtils';
import variables from '../../styles/variables';
import themeColors from '../../styles/themes/default';
import SidebarUtils from '../../libs/SidebarUtils';

const propTypes = {
/** Style for hovered state */
// eslint-disable-next-line react/forbid-prop-types
hoverStyle: PropTypes.object,

/** The ID of the report that the option is for */
reportID: PropTypes.string.isRequired,

/** Whether this option is currently in focus so we can modify its style */
isFocused: PropTypes.bool,

/** A function that is called when an option is selected. Selected option is passed as a param */
onSelectRow: PropTypes.func,

/** Toggle between compact and default view */
viewMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)),

style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),

...withLocalizePropTypes,
};

const defaultProps = {
hoverStyle: styles.sidebarLinkHover,
viewMode: 'default',
onSelectRow: () => {},
isFocused: false,
style: null,
};

const OptionRowLHN = (props) => {
const optionItem = SidebarUtils.getOptionData(props.reportID);
if (!optionItem) {
return null;
}

let touchableRef = null;
const textStyle = props.isFocused
? styles.sidebarLinkActiveText
: styles.sidebarLinkText;
const textUnreadStyle = optionItem.isUnread
? [textStyle, styles.sidebarLinkTextUnread] : [textStyle];
const displayNameStyle = StyleUtils.combineStyles(props.viewMode === CONST.OPTION_MODE.COMPACT
? [styles.optionDisplayName, ...textUnreadStyle, styles.optionDisplayNameCompact, styles.mr2]
: [styles.optionDisplayName, ...textUnreadStyle], props.style);
const alternateTextStyle = StyleUtils.combineStyles(props.viewMode === CONST.OPTION_MODE.COMPACT
? [textStyle, styles.optionAlternateText, styles.textLabelSupporting, styles.optionAlternateTextCompact]
Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out that on compact (focus) mode, because of the styles.optionAlternateText we had this emoji alignment issue #41367 on iOS: Native only.

: [textStyle, styles.optionAlternateText, styles.textLabelSupporting], props.style);
const contentContainerStyles = props.viewMode === CONST.OPTION_MODE.COMPACT
? [styles.flex1, styles.flexRow, styles.overflowHidden, styles.alignItemsCenter]
: [styles.flex1];
const sidebarInnerRowStyle = StyleSheet.flatten(props.viewMode === CONST.OPTION_MODE.COMPACT ? [
styles.chatLinkRowPressable,
styles.flexGrow1,
styles.optionItemAvatarNameWrapper,
styles.optionRowCompact,
styles.justifyContentCenter,
] : [
styles.chatLinkRowPressable,
styles.flexGrow1,
styles.optionItemAvatarNameWrapper,
styles.optionRow,
styles.justifyContentCenter,
]);
const hoveredBackgroundColor = props.hoverStyle && props.hoverStyle.backgroundColor
? props.hoverStyle.backgroundColor
: themeColors.sidebar;
const focusedBackgroundColor = styles.sidebarLinkActive.backgroundColor;
const isMultipleParticipant = lodashGet(optionItem, 'participantsList.length', 0) > 1;

// We only create tooltips for the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB but maybe a good thing to look into... this is kind of yellow flag since the feature breaks if you scroll to the next set of users? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was existing code, so could you create a GH so we don't forget to look into that? Would be great for Margelo to look into.

const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips((optionItem.participantsList || []).slice(0, 10), isMultipleParticipant);
const avatarTooltips = !optionItem.isChatRoom && !optionItem.isArchivedRoom ? _.pluck(displayNamesWithTooltips, 'tooltip') : undefined;

return (
<Hoverable>
{hovered => (
<TouchableOpacity
ref={el => touchableRef = el}
onPress={(e) => {
if (e) {
e.preventDefault();
}

props.onSelectRow(optionItem, touchableRef);
}}
activeOpacity={0.8}
style={[
styles.flexRow,
styles.alignItemsCenter,
styles.justifyContentBetween,
styles.sidebarLink,
styles.sidebarLinkInner,
StyleUtils.getBackgroundColorStyle(themeColors.sidebar),
props.isFocused ? styles.sidebarLinkActive : null,
hovered && !props.isFocused ? props.hoverStyle : null,
]}
>
<View accessibilityHint="Navigates to a chat" style={sidebarInnerRowStyle}>
<View
style={[
styles.flexRow,
styles.alignItemsCenter,
]}
>
{
!_.isEmpty(optionItem.icons)
&& (
optionItem.shouldShowSubscript ? (
<SubscriptAvatar
mainAvatar={optionItem.icons[0]}
secondaryAvatar={optionItem.icons[1]}
mainTooltip={optionItem.ownerEmail}
secondaryTooltip={optionItem.subtitle}
size={props.viewMode === CONST.OPTION_MODE.COMPACT ? CONST.AVATAR_SIZE.SMALL : CONST.AVATAR_SIZE.DEFAULT}
/>
) : (
<MultipleAvatars
icons={optionItem.icons}
size={props.viewMode === CONST.OPTION_MODE.COMPACT ? CONST.AVATAR_SIZE.SMALL : CONST.AVATAR_SIZE.DEFAULT}
secondAvatarStyle={[
StyleUtils.getBackgroundAndBorderStyle(themeColors.sidebar),
props.isFocused
? StyleUtils.getBackgroundAndBorderStyle(focusedBackgroundColor)
: undefined,
hovered && !props.isFocused
? StyleUtils.getBackgroundAndBorderStyle(hoveredBackgroundColor)
: undefined,
]}
avatarTooltips={optionItem.isPolicyExpenseChat ? [optionItem.subtitle] : avatarTooltips}
/>
)
)
}
<View style={contentContainerStyles}>
<DisplayNames
accessibilityLabel="Chat user display names"
fullTitle={optionItem.text}
displayNamesWithTooltips={displayNamesWithTooltips}
tooltipEnabled
numberOfLines={1}
textStyles={displayNameStyle}
shouldUseFullTitle={optionItem.isChatRoom || optionItem.isPolicyExpenseChat}
/>
{optionItem.alternateText ? (
<Text
style={alternateTextStyle}
numberOfLines={1}
accessibilityLabel="Last chat message preview"
>
{optionItem.alternateText}
</Text>
) : null}
</View>
{optionItem.descriptiveText ? (
<View style={[styles.flexWrap]}>
<Text style={[styles.textLabel]}>
{optionItem.descriptiveText}
</Text>
</View>
) : null}
{optionItem.brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR && (
<View style={[styles.alignItemsCenter, styles.justifyContentCenter]}>
<Icon
src={Expensicons.DotIndicator}
fill={colors.red}
height={variables.iconSizeSmall}
width={variables.iconSizeSmall}
/>
</View>
)}
</View>
</View>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
{optionItem.hasDraftComment && (
<View style={styles.ml2}>
<Icon src={Expensicons.Pencil} height={16} width={16} />
</View>
)}
{optionItem.hasOutstandingIOU && (
<IOUBadge iouReportID={optionItem.iouReportID} />
)}
{optionItem.isPinned && (
<View style={styles.ml2}>
<Icon src={Expensicons.Pin} height={16} width={16} />
</View>
)}
</View>
</TouchableOpacity>
)}
</Hoverable>
);
};

OptionRowLHN.propTypes = propTypes;
OptionRowLHN.defaultProps = defaultProps;
OptionRowLHN.displayName = 'OptionRowLHN';

export default withLocalize(OptionRowLHN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is React.memo is missing is it on purpose?

Copy link
Contributor Author

@tgolen tgolen Sep 5, 2022

Choose a reason for hiding this comment

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

Sort of, yes. I didn't purposefully avoid it, but could help me understand how it helps here? There is a lot that I don't know, so I'm just looking to learn more about it!

Copy link
Contributor

Choose a reason for hiding this comment

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

FlatList rerenders items onScroll event also when you add more items. Without memo items will be re-rendered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern is the option row depends on 6 different collections of data from Onyx in order to render, and it should re-render if any of those change (like the name of a report participant for example). Since the only prop being passed to this component is reportID, then the component has no other way of re-rendering when some of the Onyx data changes unless OptionRowLHN also subscribes to the same withOnyx() data. I was really trying to avoid having possibly thousands of these OptionRowLHN components all connected to Onyx individually as I don't know what it would do for performance.

Ultimately, if it's very very cheap to render OptionRowLHN then the benefit of memoizing it becomes less.

Can you think of anything else that would help?

Loading