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

fix: regression 23735 #24619

Closed
wants to merge 9 commits into from
Closed

fix: regression 23735 #24619

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b1c1d0e
fix: regression 23735
tienifr Aug 16, 2023
f833432
change to use state
tienifr Aug 17, 2023
c28f4c6
fix prettier
tienifr Aug 17, 2023
b8c3213
fix: regression
tienifr Aug 18, 2023
affed48
fix: clean code
tienifr Aug 18, 2023
58f510c
Merge branch 'main' into fix/regression-23735
tienifr Aug 22, 2023
a474f7d
add explanation
tienifr Aug 22, 2023
c810c83
update comments
tienifr Aug 23, 2023
ea36c0e
merge main
tienifr Aug 25, 2023
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
19 changes: 17 additions & 2 deletions src/pages/home/sidebar/SidebarLinks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ class SidebarLinks extends React.PureComponent {
if (this.props.isSmallScreenWidth) {
App.confirmReadyToOpenApp();
}

this.state = {
firstLoadedReportID: '',
};
}

componentDidMount() {
Expand Down Expand Up @@ -126,6 +130,17 @@ class SidebarLinks extends React.PureComponent {
ReportActionContextMenu.hideContextMenu(false);
}

componentDidUpdate() {
const firstLoadedReportID = this.props.optionListItems[0];
if (!this.props.isLoading || this.state.firstLoadedReportID || !firstLoadedReportID) {
return;
}

this.setState({
firstLoadedReportID,
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are better approaches than this. Currently, data might be changed from props and will not be updated. Also, the state is set after the first or second render. I propose to move this value to const before the render method.

const firstLoadedReportID = this.props.optionListItems[0] || ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

move this value to const before the render method.

sorry I don't really understand your mean. How can we do this because the reports will be empty at the first render right? Can you share the diff? @ArekChr

Copy link
Contributor

Choose a reason for hiding this comment

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

diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js
index 9d74c63d4a..b8ae8da846 100644
--- a/src/pages/home/sidebar/SidebarLinks.js
+++ b/src/pages/home/sidebar/SidebarLinks.js
@@ -130,17 +130,6 @@ class SidebarLinks extends React.PureComponent {
         ReportActionContextMenu.hideContextMenu(false);
     }
 
-    componentDidUpdate() {
-        const firstLoadedReportID = this.props.optionListItems[0];
-        if (!this.props.isLoading || this.state.firstLoadedReportID || !firstLoadedReportID) {
-            return;
-        }
-
-        this.setState({
-            firstLoadedReportID,
-        });
-    }
-
     componentWillUnmount() {
         SidebarUtils.resetIsSidebarLoadedReadyPromise();
         if (this.unsubscribeEscapeKey) {
@@ -190,6 +179,8 @@ class SidebarLinks extends React.PureComponent {
     render() {
         const viewMode = this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? CONST.OPTION_MODE.COMPACT : CONST.OPTION_MODE.DEFAULT;
 
+        const firstLoadedReportID = this.props.optionListItems[0] || '';
+
         return (
             <View style={[styles.flex1, styles.h100]}>
                 <View
@@ -243,9 +234,9 @@ class SidebarLinks extends React.PureComponent {
                 </View>
                 {this.props.isLoading ? (
                     <>
-                        {this.state.firstLoadedReportID && (
+                        {firstLoadedReportID && (
                             <OptionRowLHNData
-                                reportID={this.state.firstLoadedReportID}
+                                reportID={firstLoadedReportID}
                                 viewMode={viewMode}
                                 shouldDisableFocusOptions={this.props.isSmallScreenWidth}
                                 onSelectRow={this.showReportPage}

Copy link
Contributor

Choose a reason for hiding this comment

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

But this.props.isLoading is removed from the logic. Still determining if this is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think your solution will work because this.props.optionListItems[0] can be changed when the isLoading is true

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see now, good then


componentWillUnmount() {
SidebarUtils.resetIsSidebarLoadedReadyPromise();
if (this.unsubscribeEscapeKey) {
Expand Down Expand Up @@ -228,9 +243,9 @@ class SidebarLinks extends React.PureComponent {
</View>
{this.props.isLoading ? (
<>
{this.props.optionListItems[0] && (
{this.state.firstLoadedReportID && (
<OptionRowLHNData
reportID={this.props.optionListItems[0]}
reportID={this.state.firstLoadedReportID}
viewMode={viewMode}
shouldDisableFocusOptions={this.props.isSmallScreenWidth}
onSelectRow={this.showReportPage}
Expand Down