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

Remove class component from ReportActionsList #16613

Merged
merged 12 commits into from
Apr 12, 2023
210 changes: 102 additions & 108 deletions src/pages/home/report/ReportActionsList.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import PropTypes from 'prop-types';
import React from 'react';
import {Animated} from 'react-native';
import React, {useCallback, useEffect, useState} from 'react';
import Animated, {useSharedValue, useAnimatedStyle, withTiming} from 'react-native-reanimated';
import _ from 'underscore';
import InvertedFlatList from '../../../components/InvertedFlatList';
import withDrawerState, {withDrawerPropTypes} from '../../../components/withDrawerState';
Expand All @@ -17,11 +17,16 @@ import participantPropTypes from '../../../components/participantPropTypes';
import * as ReportActionsUtils from '../../../libs/ReportActionsUtils';
import reportActionPropTypes from './reportActionPropTypes';
import CONST from '../../../CONST';
import * as StyleUtils from '../../../styles/StyleUtils';
import reportPropTypes from '../../reportPropTypes';
import networkPropTypes from '../../../components/networkPropTypes';
import withLocalize from '../../../components/withLocalize';

// This is a workaround to a reanimated issue -> https://github.com/software-mansion/react-native-reanimated/issues/3355
if (process.browser) {
// eslint-disable-next-line no-underscore-dangle
window._frameTimestamp = null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like software-mansion/react-native-reanimated#3355 was fixed in software-mansion/react-native-reanimated@2fd0da8 which was released in 3.0.0, however you're still using 3.0.0-rc.10. It's okay to keep this workaround as for now but once you upgrade to 3.0.0+ (currently the latest version is 3.0.2) you should be safe to get rid of it.

Copy link

@johkade johkade Apr 2, 2023

Choose a reason for hiding this comment

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

Hey there :-)
Using reanimated@3.0.2 I still seem to have this problem with next.js unfortunately.
next.js version is 12.1.5
The workaround from above still seems to work.


const propTypes = {
/** Position of the "New" line marker */
newMarkerReportActionID: PropTypes.string,
Expand Down Expand Up @@ -64,54 +69,46 @@ const defaultProps = {
isLoadingMoreReportActions: false,
};

class ReportActionsList extends React.Component {
constructor(props) {
super(props);
this.renderItem = this.renderItem.bind(this);
this.keyExtractor = this.keyExtractor.bind(this);

this.state = {
fadeInAnimation: new Animated.Value(0),
skeletonViewHeight: 0,
};
}

componentDidMount() {
this.fadeIn();
}

fadeIn() {
Animated.timing(this.state.fadeInAnimation, {
toValue: 1,
duration: 100,
useNativeDriver: true,
}).start();
}
/**
* Create a unique key for each action in the FlatList.
* We use the reportActionID that is a string representation of a random 64-bit int, which should be
* random enough to avoid collisions
* @param {Object} item
* @param {Object} item.action
* @return {String}
*/
function keyExtractor(item) {
return item.reportActionID;
}
Comment on lines +74 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this function is defined outside ReportActionsList Wouldn't it be better to define it inside and useCallback with empty dependency array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had some conversation here about it: https://expensify.slack.com/archives/C01GTK53T8Q/p1680096510744619

I don't have a strong preference though and just going with what we decided there. Can you think of some different or better arguments on why to do what you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just thought that since keyExtractor was a part of the class component it should still be a part of the functional component but maybe having that function a part of the class component was a "mistake" in the first place.

I think having the function defined outside is slightly better e.g. If we render 100 instances of that component all will use the same function, where if we defined it inside each will use it's own function (100 functions that are all the same).


const ReportActionsList = (props) => {
const opacity = useSharedValue(0);
const animatedStyles = useAnimatedStyle(() => ({
opacity: withTiming(opacity.value, {duration: 100}),
}));
useEffect(() => {
opacity.value = 1;
}, []);
const [skeletonViewHeight, setSkeletonViewHeight] = useState(0);

/**
* Calculates the ideal number of report actions to render in the first render, based on the screen height and on
* the height of the smallest report action possible.
* @return {Number}
*/
calculateInitialNumToRender() {
const calculateInitialNumToRender = useCallback(() => {
const minimumReportActionHeight = styles.chatItem.paddingTop + styles.chatItem.paddingBottom
+ variables.fontSizeNormalHeight;
const availableHeight = this.props.windowHeight
const availableHeight = props.windowHeight
- (CONST.CHAT_FOOTER_MIN_HEIGHT + variables.contentHeaderHeight);
return Math.ceil(availableHeight / minimumReportActionHeight);
}
}, [props.windowHeight]);

/**
* Create a unique key for each action in the FlatList.
* We use the reportActionID that is a string representation of a random 64-bit int, which should be
* random enough to avoid collisions
* @param {Object} item
* @param {Object} item.action
* @return {String}
*/
keyExtractor(item) {
return item.reportActionID;
}
const report = props.report;
const hasOutstandingIOU = props.report.hasOutstandingIOU;
const newMarkerReportActionID = props.newMarkerReportActionID;
const sortedReportActions = props.sortedReportActions;
const mostRecentIOUReportActionID = props.mostRecentIOUReportActionID;

/**
* Do not move this or make it an anonymous function it is a method
Expand All @@ -124,87 +121,84 @@ class ReportActionsList extends React.Component {
*
* @returns {React.Component}
*/
renderItem({
const renderItem = useCallback(({
item: reportAction,
index,
}) {
}) => {
// When the new indicator should not be displayed we explicitly set it to null
const shouldDisplayNewMarker = reportAction.reportActionID === this.props.newMarkerReportActionID;
const shouldDisplayNewMarker = reportAction.reportActionID === newMarkerReportActionID;
return (
<ReportActionItem
report={this.props.report}
report={report}
action={reportAction}
displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(this.props.sortedReportActions, index)}
displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(sortedReportActions, index)}
shouldDisplayNewMarker={shouldDisplayNewMarker}
isMostRecentIOUReportAction={reportAction.reportActionID === this.props.mostRecentIOUReportActionID}
hasOutstandingIOU={this.props.report.hasOutstandingIOU}
isMostRecentIOUReportAction={reportAction.reportActionID === mostRecentIOUReportActionID}
hasOutstandingIOU={hasOutstandingIOU}
index={index}
/>
);
}

render() {
// Native mobile does not render updates flatlist the changes even though component did update called.
// To notify there something changes we can use extraData prop to flatlist
const extraData = (!this.props.isDrawerOpen && this.props.isSmallScreenWidth) ? this.props.newMarkerReportActionID : undefined;
const shouldShowReportRecipientLocalTime = ReportUtils.canShowReportRecipientLocalTime(this.props.personalDetails, this.props.report);
return (
<Animated.View style={[StyleUtils.fade(this.state.fadeInAnimation), styles.flex1]}>
<InvertedFlatList
accessibilityLabel={this.props.translate('sidebarScreen.listOfChatMessages')}
ref={ReportScrollManager.flatListRef}
data={this.props.sortedReportActions}
renderItem={this.renderItem}
contentContainerStyle={[
styles.chatContentScrollView,
shouldShowReportRecipientLocalTime && styles.pt0,
]}
keyExtractor={this.keyExtractor}
initialRowHeight={32}
initialNumToRender={this.calculateInitialNumToRender()}
onEndReached={this.props.loadMoreChats}
onEndReachedThreshold={0.75}
ListFooterComponent={() => {
if (this.props.report.isLoadingMoreReportActions) {
return (
<ReportActionsSkeletonView
containerHeight={CONST.CHAT_SKELETON_VIEW.AVERAGE_ROW_HEIGHT * 3}
/>
);
}

// Make sure the oldest report action loaded is not the first. This is so we do not show the
// skeleton view above the created action in a newly generated optimistic chat or one with not
// that many comments.
const lastReportAction = _.last(this.props.sortedReportActions) || {};
if (this.props.report.isLoadingReportActions && lastReportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED) {
return (
<ReportActionsSkeletonView
containerHeight={this.state.skeletonViewHeight}
animate={!this.props.network.isOffline}
/>
);
}

return null;
}}
keyboardShouldPersistTaps="handled"
onLayout={(event) => {
this.setState({
skeletonViewHeight: event.nativeEvent.layout.height,
});
this.props.onLayout(event);
}}
onScroll={this.props.onScroll}
extraData={extraData}
/>
</Animated.View>
);
}
}
}, [report, hasOutstandingIOU, newMarkerReportActionID, sortedReportActions, mostRecentIOUReportActionID]);

// Native mobile does not render updates flatlist the changes even though component did update called.
// To notify there something changes we can use extraData prop to flatlist
const extraData = (!props.isDrawerOpen && props.isSmallScreenWidth) ? props.newMarkerReportActionID : undefined;
const shouldShowReportRecipientLocalTime = ReportUtils.canShowReportRecipientLocalTime(props.personalDetails, props.report);
return (
<Animated.View style={[animatedStyles, styles.flex1]}>
<InvertedFlatList
accessibilityLabel={props.translate('sidebarScreen.listOfChatMessages')}
ref={ReportScrollManager.flatListRef}
data={props.sortedReportActions}
renderItem={renderItem}
contentContainerStyle={[
styles.chatContentScrollView,
shouldShowReportRecipientLocalTime && styles.pt0,
]}
keyExtractor={keyExtractor}
initialRowHeight={32}
initialNumToRender={calculateInitialNumToRender()}
onEndReached={props.loadMoreChats}
onEndReachedThreshold={0.75}
ListFooterComponent={() => {
if (props.report.isLoadingMoreReportActions) {
return (
<ReportActionsSkeletonView
containerHeight={CONST.CHAT_SKELETON_VIEW.AVERAGE_ROW_HEIGHT * 3}
/>
);
}

// Make sure the oldest report action loaded is not the first. This is so we do not show the
// skeleton view above the created action in a newly generated optimistic chat or one with not
// that many comments.
const lastReportAction = _.last(props.sortedReportActions) || {};
if (props.report.isLoadingReportActions && lastReportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED) {
return (
<ReportActionsSkeletonView
containerHeight={skeletonViewHeight}
animate={!props.network.isOffline}
/>
);
}

return null;
}}
keyboardShouldPersistTaps="handled"
onLayout={(event) => {
setSkeletonViewHeight(event.nativeEvent.layout.height);
props.onLayout(event);
}}
onScroll={props.onScroll}
extraData={extraData}
/>
</Animated.View>
);
};

ReportActionsList.propTypes = propTypes;
ReportActionsList.defaultProps = defaultProps;
ReportActionsList.displayName = 'ReportActionsList';

export default compose(
withDrawerState,
Expand Down