-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix showing NotFound page when offline #31257
Conversation
@jjcoffee PR is ready! I had a little trouble with my internet connection, please review the PR I'll complete the screenshot later. |
@hungvu193 Are you able to add the rest of the screenshots? |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-native-2023-12-14_17.27.39.mp4Android: mWeb Chromeandroid-chrome-2023-12-14_17.35.26.mp4iOS: Nativeios-native-2023-12-14_17.49.44.mp4iOS: mWeb Safariios-safari-2023-12-14_17.46.21.mp4MacOS: Chrome / Safaridesktop-chrome-2023-12-14_17.03.26.mp4MacOS: Desktopdesktop-app-2023-12-14_17.22.18.mp4 |
This comment was marked as resolved.
This comment was marked as resolved.
I couldn't reproduce it, can you provide the Tests steps? @jjcoffee |
@hungvu193 If you go offline whilst the Can you also look into this issue? Thanks! |
Since we moved the FullScreenLoadingIndicator to |
@hungvu193 Sorry, but I think we need to implement a correct fix here, regardless of what the other PR has implemented (the way the loading screen displays should have been a regression IMO). Based on your proposal, we would have shown the loading indicator within the screen, below the header, so I think let's go ahead and implement that here and keep bad UX out of the app 😄 |
I think that's ok to fix it, however other HOCs also used the same approach by using FullScreenLoadingIndicator inside. |
@hungvu193 Can you give some examples where other HOCs do the same thing? |
Yeah App/src/pages/home/report/withReportOrNotFound.tsx Lines 50 to 56 in 72e73ae
and App/src/pages/home/report/withReportAndReportActionOrNotFound.js Lines 109 to 117 in 72e73ae
|
Are there some steps to get one of these to show? I'm curious where this pattern actually shows up on the FE. |
Also any thoughts on the issue when going offline? |
@hungvu193 Friendly bump on the above. |
Oops sorry, I've been so busy on my jobs. I'll give you a solution tomorrow |
@hungvu193 Any update on this? |
@hungvu193 Can you let us know if you're able to complete the work on this? No worries at all if not, it's just good to know! |
Oh sorry! I've been out sick last couple of weeks and can't be available next few days. Do you have any suggesstion for the solutions? I'll take a look when I feel better. |
@jjcoffee I updated the code! |
That makes sense, thanks! So then I think the final thing to deal with here is the issue I mentioned previously, here's a video demonstrating it: issue-2023-12-14_14.44.06.mp4The key is to go offline before the |
I see 😮💨 . Seems some logics were added in the meantime that will navigate the screen to edit note page if our notes are failed to load. Let me take a look. |
On main I get a not found page under the same circumstance, so I think it's within scope to fix. |
Thanks for testing it. I'm on it 🫡 |
@jjcoffee I believe these lines were added weeks ago caused the issue you mentioned. Lines 4310 to 4314 in 78ad3f9
It's checking if the note is empty then navigating to edit note page. So I think it's kind of out of scope for this PR. You can try to remove these lines and test again, here's the result: Screen.Recording.2023-12-14.at.22.38.36.mov |
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.
Tests well and LGTM!
Friendly bump @puneetlath |
Sorry to be so late on this. Can you explain why we are creating a new loading page instead of using the |
@puneetlath I think you mean |
@puneetlath There's a bit more context here - it's mainly useful to have the header display on mobile as otherwise there's no way to navigate back from the loading screen. |
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.
One small comment. Otherwise seems good to me.
src/pages/LoadingPage.js
Outdated
onBackButtonPress: undefined, | ||
}; | ||
|
||
// eslint-disable-next-line rulesdir/no-negated-variables |
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.
Why is this needed?
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.
I see. It seems redundant. Updated!
@jjcoffee the reviewer checklist check is also failing. |
Checklist updated to the latest version! |
Ok great. Just one outstanding question for you then @hungvu193. |
Co-authored-by: Joel Davies <[email protected]>
Cool. I've just updated the code. |
✋ 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/puneetlath in version: 1.4.24-0 🚀
|
👋 I think this PR may have caused this deploy blocker #34270. Would you mind taking a look @jjcoffee @puneetlath ? |
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.24-3 🚀
|
Details
Fix showing NotFound page when offline
Fixed Issues
$ #30246
PROPOSAL: #30246 (comment)
Tests
Offline tests
Same as Tests.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screen.Recording.2023-11-13.at.14.13.03.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
Screen.Recording.2023-11-13.at.17.48.44.mov
iOS
Android