-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[No QA] [Performance] Approximate the ideal number of reports to render on first render #4371
Changes from 1 commit
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 |
---|---|---|
@@ -1,16 +1,25 @@ | ||
import React, {forwardRef} from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import BaseInvertedFlatList from './BaseInvertedFlatList'; | ||
|
||
export default forwardRef((props, ref) => ( | ||
const propTypes = { | ||
/** The initial number of items to render */ | ||
initialNumToRender: PropTypes.number.isRequired, | ||
}; | ||
|
||
const InvertedFlatList = forwardRef((props, ref) => ( | ||
<BaseInvertedFlatList | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
ref={ref} | ||
initialNumToRender={20} | ||
initialNumToRender={props.initialNumToRender} | ||
|
||
// Remove clipped subviews helps prevent avatars and attachments from eating up excess memory on Android. When | ||
// we run out of memory images stop appearing without any warning. | ||
// eslint-disable-next-line react/jsx-props-no-multi-spaces | ||
shouldRemoveClippedSubviews | ||
/> | ||
)); | ||
|
||
InvertedFlatList.propTypes = propTypes; | ||
export default InvertedFlatList; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,20 @@ | ||
import React, {forwardRef} from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import BaseInvertedFlatList from './BaseInvertedFlatList'; | ||
|
||
export default forwardRef((props, ref) => ( | ||
const propTypes = { | ||
/** The initial number of items to render */ | ||
initialNumToRender: PropTypes.number.isRequired, | ||
}; | ||
|
||
const InvertedFlatList = forwardRef((props, ref) => ( | ||
<BaseInvertedFlatList | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
ref={ref} | ||
initialNumToRender={20} | ||
initialNumToRender={props.initialNumToRender} | ||
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. Probably we can just remove this since each platform version is getting the prop passed via I'm not sure it should be a required prop as we can still fallback to the default value in cases where we are not getting more specific. Other things may use the inverted 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. 🤦♂️ Thanks for the catch! This really is unnecessary. Changed the code for all platforms. |
||
/> | ||
)); | ||
|
||
InvertedFlatList.propTypes = propTypes; | ||
export default InvertedFlatList; |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -36,6 +36,7 @@ import withLocalize, {withLocalizePropTypes} from '../../../components/withLocal | |||||||||
import ReportActionComposeFocusManager from '../../../libs/ReportActionComposeFocusManager'; | ||||||||||
import {contextMenuRef} from './ContextMenu/ReportActionContextMenu'; | ||||||||||
import PopoverReportActionContextMenu from './ContextMenu/PopoverReportActionContextMenu'; | ||||||||||
import variables from '../../../styles/variables'; | ||||||||||
|
||||||||||
const propTypes = { | ||||||||||
/** The ID of the report actions will be created for */ | ||||||||||
|
@@ -258,6 +259,20 @@ class ReportActionsView extends React.Component { | |||||||||
}); | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Calculates the ideal number of reports to render in the first render, based on the screen height and on | ||||||||||
* the height of the smallest report possible. | ||||||||||
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.
Suggested change
|
||||||||||
* @return {Number} | ||||||||||
*/ | ||||||||||
calculateInitialNumToRender() { | ||||||||||
const smallestReport = styles.chatItem.paddingTop + styles.chatItem.paddingBottom | ||||||||||
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. name |
||||||||||
+ variables.fontSizeNormalHeight; | ||||||||||
const availableHeight = this.props.windowHeight | ||||||||||
- (styles.chatItemCompose.minHeight + variables.contentHeaderHeight); | ||||||||||
return Math.ceil(availableHeight / smallestReport); | ||||||||||
} | ||||||||||
|
||||||||||
|
||||||||||
/** | ||||||||||
* Updates and sorts the report actions by sequence number | ||||||||||
* | ||||||||||
|
@@ -420,6 +435,7 @@ class ReportActionsView extends React.Component { | |||||||||
contentContainerStyle={styles.chatContentScrollView} | ||||||||||
keyExtractor={this.keyExtractor} | ||||||||||
initialRowHeight={32} | ||||||||||
initialNumToRender={this.calculateInitialNumToRender()} | ||||||||||
onEndReached={this.loadMoreChats} | ||||||||||
onEndReachedThreshold={0.75} | ||||||||||
ListFooterComponent={this.state.isLoadingMoreChats | ||||||||||
|
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.
We can revert all these changes in this file I think?