-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Separate ReportScreenContext to prevent cyclic re-renders #25948
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
Changes from 3 commits
3be5e69
6125c38
be7ea56
caed994
37926ee
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 |
---|---|---|
|
@@ -33,7 +33,7 @@ import getIsReportFullyVisible from '../../libs/getIsReportFullyVisible'; | |
import MoneyRequestHeader from '../../components/MoneyRequestHeader'; | ||
import MoneyReportHeader from '../../components/MoneyReportHeader'; | ||
import * as ComposerActions from '../../libs/actions/Composer'; | ||
import ReportScreenContext from './ReportScreenContext'; | ||
import {ActionListContext, ReactionListContext} from './ReportScreenContext'; | ||
import TaskHeaderActionButton from '../../components/TaskHeaderActionButton'; | ||
import DragAndDropProvider from '../../components/DragAndDrop/Provider'; | ||
|
||
|
@@ -276,111 +276,107 @@ class ReportScreen extends React.Component { | |
} | ||
|
||
return ( | ||
<ReportScreenContext.Provider | ||
value={{ | ||
flatListRef: this.flatListRef, | ||
reactionListRef: this.reactionListRef, | ||
}} | ||
> | ||
<ScreenWrapper | ||
style={screenWrapperStyle} | ||
shouldEnableKeyboardAvoidingView={isTopMostReportId} | ||
> | ||
<FullPageNotFoundView | ||
shouldShow={(!this.props.report.reportID && !this.props.report.isLoadingReportActions && !isLoading) || shouldHideReport} | ||
subtitleKey="notFound.noAccess" | ||
shouldShowCloseButton={false} | ||
shouldShowBackButton={this.props.isSmallScreenWidth} | ||
onBackButtonPress={Navigation.goBack} | ||
shouldShowLink={false} | ||
<ActionListContext.Provider value={this.flatListRef}> | ||
<ReactionListContext.Provider value={this.reactionListRef}> | ||
<ScreenWrapper | ||
style={screenWrapperStyle} | ||
shouldEnableKeyboardAvoidingView={isTopMostReportId} | ||
> | ||
<OfflineWithFeedback | ||
pendingAction={addWorkspaceRoomOrChatPendingAction} | ||
errors={addWorkspaceRoomOrChatErrors} | ||
shouldShowErrorMessages={false} | ||
needsOffscreenAlphaCompositing | ||
<FullPageNotFoundView | ||
shouldShow={(!this.props.report.reportID && !this.props.report.isLoadingReportActions && !isLoading) || shouldHideReport} | ||
subtitleKey="notFound.noAccess" | ||
shouldShowCloseButton={false} | ||
shouldShowBackButton={this.props.isSmallScreenWidth} | ||
onBackButtonPress={Navigation.goBack} | ||
shouldShowLink={false} | ||
> | ||
{headerView} | ||
{ReportUtils.isTaskReport(this.props.report) && this.props.isSmallScreenWidth && ReportUtils.isOpenTaskReport(this.props.report) && ( | ||
<View style={[styles.borderBottom]}> | ||
<View style={[styles.appBG, styles.pl0]}> | ||
<View style={[styles.ph5, styles.pb3]}> | ||
<TaskHeaderActionButton report={this.props.report} /> | ||
<OfflineWithFeedback | ||
pendingAction={addWorkspaceRoomOrChatPendingAction} | ||
errors={addWorkspaceRoomOrChatErrors} | ||
shouldShowErrorMessages={false} | ||
needsOffscreenAlphaCompositing | ||
> | ||
{headerView} | ||
{ReportUtils.isTaskReport(this.props.report) && this.props.isSmallScreenWidth && ReportUtils.isOpenTaskReport(this.props.report) && ( | ||
<View style={[styles.borderBottom]}> | ||
<View style={[styles.appBG, styles.pl0]}> | ||
<View style={[styles.ph5, styles.pb3]}> | ||
<TaskHeaderActionButton report={this.props.report} /> | ||
</View> | ||
</View> | ||
</View> | ||
</View> | ||
)} | ||
</OfflineWithFeedback> | ||
{Boolean(this.props.accountManagerReportID) && ReportUtils.isConciergeChatReport(this.props.report) && this.state.isBannerVisible && ( | ||
<Banner | ||
containerStyles={[styles.mh4, styles.mt4, styles.p4, styles.bgDark]} | ||
textStyles={[styles.colorReversed]} | ||
text={this.props.translate('reportActionsView.chatWithAccountManager')} | ||
onClose={this.dismissBanner} | ||
onPress={this.chatWithAccountManager} | ||
shouldShowCloseButton | ||
/> | ||
)} | ||
<DragAndDropProvider isDisabled={!this.isReportReadyForDisplay()}> | ||
<View | ||
style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]} | ||
onLayout={(event) => { | ||
// Rounding this value for comparison because they can look like this: 411.9999694824219 | ||
const skeletonViewContainerHeight = Math.round(event.nativeEvent.layout.height); | ||
|
||
// Only set state when the height changes to avoid unnecessary renders | ||
if (reportActionsListViewHeight === skeletonViewContainerHeight) return; | ||
|
||
// The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it | ||
// takes up so we can set the skeleton view container height. | ||
if (skeletonViewContainerHeight === 0) { | ||
return; | ||
} | ||
reportActionsListViewHeight = skeletonViewContainerHeight; | ||
this.setState({skeletonViewContainerHeight}); | ||
}} | ||
> | ||
{this.isReportReadyForDisplay() && !isLoadingInitialReportActions && !isLoading && ( | ||
<ReportActionsView | ||
reportActions={this.props.reportActions} | ||
report={this.props.report} | ||
isComposerFullSize={this.props.isComposerFullSize} | ||
parentViewHeight={this.state.skeletonViewContainerHeight} | ||
policy={policy} | ||
/> | ||
)} | ||
|
||
{/* Note: The report should be allowed to mount even if the initial report actions are not loaded. If we prevent rendering the report while they are loading then | ||
we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */} | ||
{(!this.isReportReadyForDisplay() || isLoadingInitialReportActions || isLoading) && ( | ||
<ReportActionsSkeletonView containerHeight={this.state.skeletonViewContainerHeight} /> | ||
)} | ||
|
||
{this.isReportReadyForDisplay() && ( | ||
<> | ||
<ReportFooter | ||
pendingAction={addWorkspaceRoomOrChatPendingAction} | ||
isOffline={this.props.network.isOffline} | ||
</OfflineWithFeedback> | ||
{Boolean(this.props.accountManagerReportID) && ReportUtils.isConciergeChatReport(this.props.report) && this.state.isBannerVisible && ( | ||
<Banner | ||
containerStyles={[styles.mh4, styles.mt4, styles.p4, styles.bgDark]} | ||
textStyles={[styles.colorReversed]} | ||
text={this.props.translate('reportActionsView.chatWithAccountManager')} | ||
onClose={this.dismissBanner} | ||
onPress={this.chatWithAccountManager} | ||
shouldShowCloseButton | ||
/> | ||
)} | ||
<DragAndDropProvider isDisabled={!this.isReportReadyForDisplay()}> | ||
<View | ||
style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]} | ||
onLayout={(event) => { | ||
// Rounding this value for comparison because they can look like this: 411.9999694824219 | ||
const skeletonViewContainerHeight = Math.round(event.nativeEvent.layout.height); | ||
|
||
// Only set state when the height changes to avoid unnecessary renders | ||
if (reportActionsListViewHeight === skeletonViewContainerHeight) return; | ||
|
||
// The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it | ||
// takes up so we can set the skeleton view container height. | ||
if (skeletonViewContainerHeight === 0) { | ||
return; | ||
} | ||
reportActionsListViewHeight = skeletonViewContainerHeight; | ||
this.setState({skeletonViewContainerHeight}); | ||
}} | ||
> | ||
{this.isReportReadyForDisplay() && !isLoadingInitialReportActions && !isLoading && ( | ||
<ReportActionsView | ||
reportActions={this.props.reportActions} | ||
report={this.props.report} | ||
isComposerFullSize={this.props.isComposerFullSize} | ||
onSubmitComment={this.onSubmitComment} | ||
policies={this.props.policies} | ||
policy={policy} | ||
/> | ||
</> | ||
)} | ||
)} | ||
|
||
{!this.isReportReadyForDisplay() && ( | ||
<ReportFooter | ||
shouldDisableCompose | ||
isOffline={this.props.network.isOffline} | ||
/> | ||
)} | ||
</View> | ||
</DragAndDropProvider> | ||
</FullPageNotFoundView> | ||
</ScreenWrapper> | ||
</ReportScreenContext.Provider> | ||
{/* Note: The report should be allowed to mount even if the initial report actions are not loaded. If we prevent rendering the report while they are loading then | ||
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 you please clarify in this comment what "the report" is? Is it:
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. All these are part of linting (the component it is now wrapped into a new context provider), I have not changed or touched the internal logic. I can do it if you want, but it falls out of the scope of the performance improvements 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. Aren't you lucky that you stumbled into this code? 😀 I guess it's not a blocker for this particular PR, but I am always happy to see it when people leave the code better than how they found it. 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. ok, I will take care of it then :) |
||
we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */} | ||
{(!this.isReportReadyForDisplay() || isLoadingInitialReportActions || isLoading) && ( | ||
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. I think making this a ternary with the block above it will improve readability. It makes it easier to understand that the intention is that either one or the other should be displayed, never both. 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. All these are part of linting (the component it is now wrapped into a new context provider), I have not changed or touched the internal logic. I can do it if you want, but it falls out of the scope of the performance improvements |
||
<ReportActionsSkeletonView containerHeight={this.state.skeletonViewContainerHeight} /> | ||
)} | ||
|
||
{this.isReportReadyForDisplay() && ( | ||
<> | ||
<ReportFooter | ||
pendingAction={addWorkspaceRoomOrChatPendingAction} | ||
isOffline={this.props.network.isOffline} | ||
reportActions={this.props.reportActions} | ||
report={this.props.report} | ||
isComposerFullSize={this.props.isComposerFullSize} | ||
onSubmitComment={this.onSubmitComment} | ||
policies={this.props.policies} | ||
/> | ||
</> | ||
)} | ||
|
||
{!this.isReportReadyForDisplay() && ( | ||
<ReportFooter | ||
shouldDisableCompose | ||
isOffline={this.props.network.isOffline} | ||
/> | ||
)} | ||
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. These should be a ternary 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. All these are part of linting (the component it is now wrapped into a new context provider), I have not changed or touched the internal logic. I can do it if you want, but it falls out of the scope of the performance improvements |
||
</View> | ||
</DragAndDropProvider> | ||
</FullPageNotFoundView> | ||
</ScreenWrapper> | ||
</ReactionListContext.Provider> | ||
</ActionListContext.Provider> | ||
); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
import {createContext} from 'react'; | ||
|
||
const ReportScreenContext = createContext(); | ||
export default ReportScreenContext; | ||
const ActionListContext = createContext(); | ||
const ReactionListContext = createContext(); | ||
|
||
export {ActionListContext, ReactionListContext}; |
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.
Please move this logic to a method outside of the
return()
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.
All these are part of linting (the component it is now wrapped into a new context provider), I have not changed or touched the internal logic. I can do it if you want, but it falls out of the scope of the performance improvements