Conversation
|
@ArekChr In this PR I want to remove |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemweb.chrome.movMobile Web - Safarimweb.safari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
|
@tienifr I'm trying to reproduce but only successfully on ios/mWeb safari/web/mWeb chrome, I can't open the deep link on native android and desktop. What steps are to reproduce it on these platforms? |
|
@ArekChr here you are: |
ArekChr
left a comment
ArekChr
left a comment
There was a problem hiding this comment.
Works as expected, LGTM!
| componentDidUpdate() { | ||
| const firstLoadedReportID = this.props.optionListItems[0]; | ||
| if (!this.props.isLoading || this.state.firstLoadedReportID || !firstLoadedReportID) { | ||
| return; | ||
| } | ||
|
|
||
| this.setState({ | ||
| firstLoadedReportID, | ||
| }); | ||
| } |
There was a problem hiding this comment.
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] || ""
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
But this.props.isLoading is removed from the logic. Still determining if this is needed.
There was a problem hiding this comment.
I don't think your solution will work because this.props.optionListItems[0] can be changed when the isLoading is true
|
@ArekChr as @aldo-expensify's suggestion, I also think
I've tested all cases:
and all cases work well. But I still need your help to check them again. Thanks very much |
|
I see, from what I see it looks like |
|
@ArekChr Sorry but I don't really understand your mean. I removed |
|
I'm okay with both, if the value is the same let's stay with Maybe not |
From my understanding, the "equivalent" to |
| return; | ||
| } | ||
| setIsFirstTimeReportLoading(isLoadingReportData); | ||
| }, [isFirstTimeReportLoading, isLoadingReportData]); |
There was a problem hiding this comment.
The code is not very intuitive about:
- Why do we initialize
isFirstTimeReportLoadingusingSessionUtils.didUserLogInDuringSession() - Why do we need/use
isFirstTimeReportLoadinginstead ofisLoadingReportData
Would be good to add some comments, on why we are doing things like we are here.
I also have to get familiarized with SessionUtils.didUserLogInDuringSession() as I'm not what does this state represent and why does it matter for the LHN
What do you mean with this? Are you saying we should revert to the implementation that was using a state?
Not sure if it answers your question.... I understand that:
I'm fine with the implementation, but I think we should definitely add more comments so this doesn't get removed by mistakes. The code is not enough in this case to understand easily why things are implemented this way. |
|
@aldo-expensify @ArekChr Thanks for your reviews. I just added some comments to explain what I did. Are they enough to understand? |
I mean that both |
| function SidebarLinksData({isFocused, allReportActions, betas, chatReports, currentReportID, insets, isLoadingReportData, isSmallScreenWidth, onLinkClick, policies, priorityMode}) { | ||
| const {translate} = useLocalize(); | ||
|
|
||
| // SessionUtils.didUserLogInDuringSession() is true means the users have just logged in successfully, the full reports are not available util the `OpenApp` API returns data. |
There was a problem hiding this comment.
| // SessionUtils.didUserLogInDuringSession() is true means the users have just logged in successfully, the full reports are not available util the `OpenApp` API returns data. | |
| // SessionUtils.didUserLogInDuringSession() Determine if this is the user's first time loading reports in the current session. | |
| // Full reports will be unavailable until the `OpenApp` API returns data for newly logged-in users. |
|
@ArekChr Can you help check again? Thanks |
|
@tienifr I think we are going to go with this PR: #25159 (comment), just letting you know so you hold on spending more time on this for now. |
3 participants
Details
Fixed Issues
$ #23735
$ #24596
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-08-16.at.17.57.58.mov
Mobile Web - Chrome
original-414BB5EF-20BB-41E6-A946-0D3A3E1785F3.mp4
Mobile Web - Safari
original-BD1DE897-397C-4749-A13C-60DD64E31C5D.mp4
Desktop
Screen.Recording.2023-08-07.at.16.38.45.mp4
iOS
Screen.Recording.2023-08-16.at.18.09.50.mov
Android
Screen.Recording.2023-08-16.at.18.21.09.mov