-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2024-01-18] [$500] Private Note - The message 'Hmm... it's not here' displays instead of Skeleton UI when viewing Private Notes in Offline Mode. #30246
Comments
Triggered auto assignment to @puneetlath ( |
Job added to Upwork: https://www.upwork.com/jobs/~01cb01f2ab30a52085 |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.The message 'Hmm... it's not here' displays instead of Skeleton UI when viewing Private Notes in Offline Mode What is the root cause of that problem?In PrivateNotesListPage.js , should shouldShow become true due to offline mode App/src/pages/PrivateNotes/PrivateNotesListPage.js Lines 127 to 133 in cc3c51c
What changes do you think we should make in order to solve the problem?We have to update the condition with !network.isOffline in the above check and show OptionsListSkeletonView when the network is offline and loading
Screen.Recording.2023-10-24.at.6.48.57.PM.movWhat alternative solutions did you explore? (Optional)NA |
@rakshitjain13 Thanks for the proposal! If we go ahead with this change, I think we'd probably want to always display the skeleton loader instead of the full screen loader otherwise coming back online there'd be an odd transition from Skeleton Loader -> Full Screen Loader -> Notes. cc @Expensify/design for feedback! |
@jjcoffee Thanks for the feedback and I think your suggestion makes more sense. Hence updated the proposal #30246 (comment) |
ProposalPlease re-state the problem that we are trying to solve in this issue.The message 'Hmm... it's not here' displays instead of Skeleton UI when viewing Private Notes in Offline Mode. What is the root cause of that problem?We currently show NotfoundPage with below condition App/src/pages/PrivateNotes/PrivateNotesListPage.js Lines 128 to 132 in 1c85ebe
Since we only load when it's not offline, NotFoundPage will be shown in this case. App/src/pages/PrivateNotes/PrivateNotesListPage.js Lines 67 to 70 in 1c85ebe
What changes do you think we should make in order to solve the problem?Create a variable call const prevIsOffline = usePrevious(network.isOffline);
const isReconnecting = prevIsOffline && !network.isOffline; Show the loading indicator or skeleton instead of blocking user from using it (similar to other places like Address or WorkspaceMembers). So we need to update shouldShow={
_.isEmpty(report.reportID) ||
(!report.isLoadingPrivateNotes && !network.isOffline && !isReconnecting && _.isEmpty(lodashGet(report, 'privateNotes', {}))) ||
ReportUtils.isArchivedRoom(report)
} {(report.isLoadingPrivateNotes && _.isEmpty(privateNotes) || ((network.isOffline || isReconnecting) && _.isEmpty(privateNotes))) ? (
<FullScreenLoadingIndicator style={[styles.flex1, styles.pRelative]} />
) : (
_.map(privateNotes, (item, index) => getMenuItem(item, index))
)} What alternative solutions did you explore? (Optional)Similar above but we will add a new variable called
After that replace the |
@tranvantoan-qn Thanks for the extra context! Looking at that issue it looks like it's expected for the |
@jjcoffee Thanks for the confirmation that |
Yeah, we're showing Fullscreenloading in other pages until we reconnect to the internet as I said in my proposal. |
@puneetlath, @jjcoffee Eep! 4 days overdue now. Issues have feelings too... |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@jjcoffee mind summarizing where we're at with this issue currently? |
So we have two pretty identical proposals, but @hungvu193's was the first to include handling for being offline and already having loaded the notes. When testing both proposals I see a brief appearance of the private-notes-coming-online-2023-11-01_17.02.21.mp4 |
@jjcoffee I think it is because when we shift from offline to online for a moment isLoadingPrivateNotes is false. After all, it is set to false in a case when you first visited or failed due to offline App/src/libs/actions/Report.js Lines 2377 to 2385 in f9d11c5
which makes the below condition true in shouldShow
If we change to true in failureData we don't
cc: @hungvu193 |
So the Another solution, change the API.read to API.write, this will make our request is persisted and we can call |
@puneetlath @jjcoffee this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
@puneetlath, @jjcoffee Eep! 4 days overdue now. Issues have feelings too... |
This issue has not been updated in over 15 days. @puneetlath, @hungvu193, @jjcoffee eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
Just waiting for @puneetlath to merge the PR. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.24-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-01-18. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@jjcoffee bump on the checklist so that we can pay tomorrow. |
Regression Test Proposal
Do we agree 👍 or 👎 |
Note that there's an outstanding regression that is waiting on @hungvu193's input. |
@hungvu193 are you planning to handle that shortly? We won't be able to pay this one out until you do. |
Cool. I'm on it. |
@puneetlath, @hungvu193, @jjcoffee Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it! |
The PR that fixes the regression is merged by the way (not hit production yet). |
Deployed to production! @puneetlath Can we update the HOLD for payment here to 2024-02-07? Thanks! |
@puneetlath, @hungvu193, @jjcoffee Huh... This is 4 days overdue. Who can take care of this? |
@puneetlath, @hungvu193, @jjcoffee 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
All paid. Thanks everyone! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.3.90.1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @tranvantoan-qn
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697131558107909
Action Performed:
Expected Result:
A Skeleton UI should be displayed, as it is a general concept applied when the data is loading.
Actual Result:
Instead of a Skeleton UI, the message "Hmm... it's not here" is displayed.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Android: Native
Android: mWeb Chrome
Android.-.Chrome.4.mov
iOS: Native
Ios.-.Native.5.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS.-.Safari.4.mov
MacOS.-.Chrome.5.mp4
MacOS: Desktop
MacOS.-.Desktop.3.mov
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: