Fix open some pages by link display page not found #18967
Conversation
melvin-bot
bot
requested review from
dangrous and
mollfpr
and removed request for
a team
if (this.props.isLoadingReportData && _.isEmpty(this.props.iouReport)) {
return <FullScreenLoadingIndicator />;
}@dukenv0307 I think we can create a wrapper component to the above logic. |
|
@mollfpr That mean wrapper component is loading component if data is loading and it is page not found component otherwhise. Is that right? |
if (props.isLoadingReportData && _.isEmpty(props.policy)) {
return <FullscreenLoadingIndicator />;
}@dukenv0307 Sorry, I mean the above logic for the workspace pages. The not found page can be from the |
|
@mollfpr So what shoud we change? |
|
@mollfpr So we should move the above logic into |
|
@dukenv0307 On second thought, I think there are advantages to current changes. So I think we are good now, I'll start the test 👍 |
src/pages/DetailsPage.js
Outdated
| @@ -113,6 +118,10 @@ class DetailsPage extends React.PureComponent { | |||
| const phoneNumber = getPhoneNumber(details); | |||
| const phoneOrEmail = isSMSLogin ? getPhoneNumber(details) : details.login; | |||
|
|
|||
| if (this.props.isLoadingReportData && _.isEmpty(login)) { | |||
| return <FullscreenLoadingIndicator />; | |||
| } | |||
There was a problem hiding this comment.
@dukenv0307 Do we need to add a loading indicator to DetailsPage? I don't see any loading indicator in my test after signing in. The page will display fine; as long the login params are provided another way, it will show not found page.
There was a problem hiding this comment.
@mollfpr Yes you right, login is get from params so that _isEmpty(login) is always false if login has in parmas.
| if (props.isLoadingReportData && _.isEmpty(props.policy)) { | ||
| return <FullscreenLoadingIndicator />; | ||
| } | ||
|
|
There was a problem hiding this comment.
Sorry for the back and forth @dukenv0307.
@mollfpr So we should move the above logic into withPolicy with workspace pages same as withReportOrNotFound?
Seems you understand what I mean yesterday, I don't know what the other things I have are.
I just try create a new HOC for accomodate the return of fullscreen loading if the policy is not provided.
export default function (WrappedComponent) {
const propTypes = {
isLoadingReportData: PropTypes.bool,
...policyPropTypes,
};
const defaultProps = {
isLoadingReportData: true,
...policyDefaultProps,
};
const WithPolicyAndFullscreenLoading = (props) => {
if (props.isLoadingReportData && _.isEmpty(props.policy)) {
return <FullscreenLoadingIndicator />;
}
const rest = _.omit(props, ['forwardedRef']);
return (
<WrappedComponent
// eslint-disable-next-line react/jsx-props-no-spreading
{...rest}
ref={props.forwardedRef}
/>
);
};
WithPolicyAndFullscreenLoading.propTypes = propTypes;
WithPolicyAndFullscreenLoading.defaultProps = defaultProps;
WithPolicyAndFullscreenLoading.displayName = `WithPolicyAndFullscreenLoading(${getComponentDisplayName(WrappedComponent)})`;
const withPolicyAndFullscreenLoading = React.forwardRef((props, ref) => (
<WithPolicyAndFullscreenLoading
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
forwardedRef={ref}
/>
));
return compose(
withPolicy,
withOnyx({
isLoadingReportData: {
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
},
}),
)(withPolicyAndFullscreenLoading);
}So we just need to replace withPolicy with withPolicyAndFullscreenLoading in the workspace pages.
There was a problem hiding this comment.
@mollfpr Cool, I update right now.
|
@mollfpr I just updated new HOC for workspace pages, and restore detail page. And I also tested again workspace pages with new HOC, it works well. Please help to check again if I miss something. |
src/pages/iou/IOUDetailsModal.js
Outdated
| if (this.props.isLoadingReportData && _.isEmpty(this.props.iouReport)) { | ||
| return <FullScreenLoadingIndicator />; | ||
| } | ||
|
|
There was a problem hiding this comment.
@dukenv0307 I think we don't need this. When I click on the IOU preview now it's redirecting to the money request report or money request thread report.
|
@mollfpr I just updated code and test steps |
|
@dukenv0307 Thanks! Overall it tests well, and the code looks good to me 👍; I'm on my way to completing the recordings. |
Reviewer Checklist
Screenshots/VideosWeb18967.Web.-.Report.mov18967.Web.-.Workspace.movMobile Web - Chrome18967.mWeb.Chrome.-.Workspace.mp418967.mWeb.Chrome.-.Report.mp4Mobile Web - Safari18967.mWeb.Safari.-.Report.mp418967.mWeb.Safari.-.Workspace.mp4Desktop18967.Desktop.-.Report.mov18967.Desktop.-.Workspace.moviOSAndroid |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/dangrous in version: 1.3.17-0 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.17-5 🚀
|
Details
Fixed Issues
$ #18828
PROPOSAL: #18828 (comment)
Tests
Offline tests
Same above
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
Screencast.from.16-05-2023.09.55.55.webm
Screencast.from.16-05-2023.09.57.23.webm
Screencast.from.16-05-2023.10.19.48.webm
Screencast.from.16-05-2023.09.59.43.webm
Screencast.from.16-05-2023.10.02.24.webm
Screencast.from.16-05-2023.10.03.44.webm
Screencast.from.16-05-2023.10.04.52.webm
Screencast.from.16-05-2023.16.01.38.webm
Mobile Web - Chrome
Record_2023-05-16-10-31-33.mp4
Record_2023-05-16-10-35-13.mp4
Record_2023-05-16-10-36-16.mp4
Record_2023-05-16-10-37-20.mp4
Record_2023-05-16-10-40-21.mp4
Record_2023-05-16-10-41-18.mp4
Record_2023-05-16-10-42-36.mp4
Record_2023-05-16-16-03-03.mp4
Mobile Web - Safari
Screen-Recording-2023-05-16-at-14.43.14-1.mp4
Screen-Recording-2023-05-16-at-14.46.20-1.mp4
Screen-Recording-2023-05-16-at-14.47.33-1.mp4
Screen.Recording.2023-05-16.at.14.49.26-1.mp4
Screen.Recording.2023-05-16.at.14.51.59-1.mp4
Screen.Recording.2023-05-16.at.14.52.56-1.mp4
Screen.Recording.2023-05-16.at.14.54.07-1.mp4
Screen.Recording.2023-05-16.at.16.55.40-1.mp4
Desktop
Cannot open with deep link because this bug
iOS
When opening app with a deep link that is different /r/:reportID, the app always opens sidebar screen after login
Android
Same IOS above
Here is example
Screencast.from.16-05-2023.16.11.37.webm