New skeleton loader for LHN #14456

bernhardoj · 2023-01-21T05:53:11Z

Details

When we sign in to the app, a full screen loading indicator will be shown while loading the app data. Now, we want to show a skeleton instead while loading the required data.

Tagging @shawnborton for the design review.

Fixed Issues

$ #12698
PROPOSAL: #12698 (comment)

Tests

Sign out if you are signed in
Sign in back to the app
Verify a skeleton is showing while loading the app data

Verify that no errors appear in the JS console

Offline tests

(I don't think we can test it when we are fully offline)

Sign out if you are signed in
Sign in back to the app
Verify a skeleton is showing while loading the app data

QA Steps

Sign out if you are signed in
Sign in back to the app
Verify a skeleton is showing while loading the app data

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-01-20.at.21.08.55.mov

Mobile Web - Chrome

329155.t.mp4

Mobile Web - Safari

Untitled.mov

Desktop

Screen.Recording.2023-01-20.at.21.46.22.mov

iOS

Screen.Recording.2023-01-20.at.21.20.17.mov

Android

329156.t.mp4

melvin-bot · 2023-01-21T05:53:19Z

@roryabraham @aimane-chnaif One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

shawnborton · 2023-01-21T18:11:20Z

Almost! It looks like the fake sender/message preview is just slightly too far away from the avatars. Take a look here:

My guess is that those lines need to be 4px closer to the avatar.

shawnborton · 2023-01-21T18:13:13Z

Let me know if you need a new svg for that. We recently made it so that the space between all avatars and messages as 12px. So we might just need to update that svg everywhere, for both LHN and messages.

bernhardoj · 2023-01-22T01:08:01Z

Yep, you are right, we need to adjust 4px closer to the avatar. Thanks for noticing that.

Updated the code.

aimane-chnaif · 2023-01-22T22:04:09Z

bug.mov

@bernhardoj there are many weird issues in this video

reportID is added at the end of other page route
Report skeleton is showing forever
crash on LHN item click

Test step:

update Network to Slow 3G
Login
Open any other page before loading data (i.e. Click FAB + Split bill quickly or press Cmd+K shortcut to open Search page)

I think all of these are related to navigation code.

bernhardoj · 2023-01-23T02:31:56Z

@aimane-chnaif Ah, I see. I think we should disable the FAB while loading, the same thing we do with search and setting button. But the search is still accessible from the keyboard shortcuts.

At first, I thought that we need to disable the shortcut too, but then I look at staging, we can open the search page while the full screen loading showing and I think the issue you see is the same which is the list is empty. So, I am not sure about this one. Should we also disable the shortcut?

aimane-chnaif · 2023-01-23T02:38:28Z

@bernhardoj I think disabling other pages is a workaround. Can we fix this in navigation level?

bernhardoj · 2023-01-23T02:55:22Z

I will try to find a safer way to update the params.

bernhardoj · 2023-01-23T04:39:42Z

I have updated the setParams function to accept a route key parameter to update the params to the specified route key, not to the currently focused route. And to get the key of the report screen, we need to access it quite deep from the navigation state.

AuthScreen -> MainDrawerNavigator -> ReportScreen (key)
state         routes[0].state        routes[0].state.key

aimane-chnaif · 2023-01-23T10:07:34Z

I'd like to get opinion from @shawnborton @roryabraham regarding user interaction on FAB menu before loading data

hide FAB icon
show FAB icon but disable user interaction on button click
show popup menu on FAB click but disable user interaction on each item click
don't prevent any user interactions and instead handle on each page
Which is the best option?

shawnborton · 2023-01-23T10:09:04Z

We've been having a similar conversation about how to handle the chat composer input when loading chats (cc @JmillsExpensify) and I think we landed with deciding that we shouldn't block the user interacting with the FAB or even navigating to the type of object they want to create from the FAB.

bernhardoj · 2023-01-23T10:39:22Z

Maybe we can show a loading indicator while loading the data like the page here

Details Page

https://user-images.githubusercontent.com/50919443/214018965-a47b721b-82c5-449c-82b8-d32e7bc8d9e4.png

and update the list state when the data is ready.

aimane-chnaif · 2023-01-23T10:43:28Z

@bernhardoj we opened discussion here so you can 👁️

bernhardoj · 2023-01-24T04:20:07Z

I have removed the disable from the button. But I just tested that we can press New Workspace from the FAB menu while loading the initial data and it will lead to duplicate workspaces with the same name. Is this also out of scope and need to be handled on a seperate issue?

329278.t.mp4

aimane-chnaif · 2023-01-24T04:29:37Z

I don't think we should allow those kinds of issues. Something like search list empty issues can be out of scope since they're already happening on production but those issues are critical and kind of regression.
@roryabraham what do you think?

bernhardoj · 2023-01-24T11:29:08Z

So, I was trying to find a way for that FAB new workspace. I realize that when we open the app, App.openApp() will be called. It will load all the initial data and set IS_LOADING_REPORT_DATA while loading it. We can use this to hide the new workspace menu until the loading is finished.

diff --git a/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js b/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
index 6fe0447ef..4520dda29 100644
--- a/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
+++ b/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
@@ -207,7 +207,7 @@ class FloatingActionButtonAndPopover extends React.Component {
                                 onSelected: () => Navigation.navigate(ROUTES.IOU_BILL),
                             },
                         ] : []),
-                        ...(!Policy.hasActiveFreePolicy(this.props.allPolicies) ? [
+                        ...(!this.props.isLoading && !Policy.hasActiveFreePolicy(this.props.allPolicies) ? [
                             {
                                 icon: Expensicons.NewWorkspace,
                                 iconWidth: 46,
@@ -247,5 +247,8 @@ export default compose(
         betas: {
             key: ONYXKEYS.BETAS,
         },
+        isLoading: {
+            key: ONYXKEYS.IS_LOADING_REPORT_DATA,
+        },
     }),
 )(FloatingActionButtonAndPopover);

With the IS_LOADING_REPORT_DATA key, this makes me think that maybe we can also use this to replace the emptiness condition of report and personal details. For example,

replace this
SidebarLink.js
const isLoading = _.isEmpty(this.props.personalDetails) || _.isEmpty(this.props.chatReports);

with the IS_LOADING_REPORT_DATA onyx key.

But the problem with this onyx key is that it will also be true when ReconnectApp is called. On Android (the device I tested), when we come from background, ReconnectApp will be called. This will make the skeleton will show again. So, maybe we can create a new key that is only set to true on OpenApp (like IS_LOADING_INITIAL_REPORT_DATA).

I am not sure whether this is the best approach or not because our current code uses the emptiness state of report/personal details and not using the IS_LOADING_REPORT_DATA.

Sorry if it's not clear.

What do you think? @aimane-chnaif @roryabraham

aimane-chnaif · 2023-03-06T20:35:06Z

@roryabraham kindly bump for re-review

roryabraham

Only one very minor blocker in boolean variable name. Other than that I think this code looks good, but I'm a bit confused about why some of the changes were needed.

src/components/ReportHeaderSkeletonView.js

src/pages/home/ReportScreen.js

src/pages/home/sidebar/SidebarLinks.js

aimane-chnaif · 2023-03-12T17:37:43Z

@roryabraham bump again for review!

aimane-chnaif · 2023-03-13T19:38:30Z

@bernhardoj please fix pull from main. It seems conflict with #15897

bernhardoj · 2023-03-14T02:34:17Z

Fixed. I also updated the freeze and animate variable to make it consistent with SidebarLinks.

aimane-chnaif · 2023-03-21T03:54:37Z

@bernhardoj please fix conflict
@roryabraham bump again for review

bernhardoj · 2023-03-21T05:59:10Z

Fixed. I also have retested the steps from this PR #15994 (which causes the conflict) and still works well.

OSBotify · 2023-03-23T07:16:24Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

bernhardoj · 2023-03-23T14:39:02Z

Cool, finally 🥳. Thanks for the review @aimane-chnaif @roryabraham @shawnborton.

I guess the next step is to follow up on some issues here #14456 (comment). Here is the list of issues that I think we need to follow up:

Empty search, new chat, new group, and probably iou participants
We already have an issue for empty data here [HOLD for payment 2023-05-22] [$4000] Not showing any data on search page when press Cmd+K shortcut while loading after login #14490. I think we can use this issue specifically for these 4 pages because all of them use the same OptionsSelector, which means the same skeleton design (maybe we can even reuse the LHN skeleton).
Empty profile page
No issue created yet
IOU participants error
I already found out the root cause of the issue of this and we need a discussion what is the best way to handle it. The decision here may remove the need for iou participants skeleton from no. 1 (or maybe not because the user can still access it through the browser address bar directly).
Empty IOU currency list
No issue created yet

aimane-chnaif · 2023-03-24T13:34:45Z

Minor console warning: (happens after login)

@bernhardoj can you investigate this?

aimane-chnaif · 2023-03-24T15:49:13Z

Minor console warning: (happens after login)

Here's discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1679665614690159 (for reference)

OSBotify · 2023-03-24T19:06:20Z

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.2.89-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-03-27T10:33:02Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.89-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

hoangzinh · 2023-12-18T16:15:44Z

src/components/LHNSkeletonView.js

+                    this.generateSkeletonViewItems(numItems);
+                }}
+            >
+                <View>{this.state.skeletonViewItems}</View>


Hi @bernhardoj, coming from #33163, Is there any specific reason that we didn't render SkeletonViewContentLoader directly via an array map function here, but pre-calculate and stored in this.state.skeletonViewItems in onLayout callback?

Changed it based on this suggestion. I think it's just for optimization.

Thanks @bernhardoj

Hi @roryabraham, do you mind sharing the reason why you prefer to generate skeletonViewItems and store it in the component's state? In a first glance, I think it's because of optimization as @bernhardoj said above but I usually see the pattern where we generate a list/array of items (with a key) in component render function, leaving the rendering optimization part for React. Thanks

^ just in case you missed this message @roryabraham

Answered here

bernhardoj added 4 commits January 20, 2023 19:30

show skeleton instead of activity indicator while loading the app

0c38b1b

only show back button on small screen

a430364

move around the loading state

ea87754

fix lint

5a93cec

bernhardoj requested a review from a team as a code owner January 21, 2023 05:53

melvin-bot bot requested review from aimane-chnaif and roryabraham and removed request for a team January 21, 2023 05:53

fix lint

3574120

shawnborton self-requested a review January 21, 2023 18:11

update skeleton line position

60011ae

set the params to the specific route

f2f5c25

fix lint

27c99ef

undisable header buttons

efbfd4d

add comment

3a660a5

roryabraham requested changes Mar 6, 2023

View reviewed changes

src/components/ReportHeaderSkeletonView.js Show resolved Hide resolved

src/pages/home/ReportScreen.js Outdated Show resolved Hide resolved

src/pages/home/ReportScreen.js Show resolved Hide resolved

src/pages/home/sidebar/SidebarLinks.js Outdated Show resolved Hide resolved

change variable name

829b20e

bernhardoj added 2 commits March 13, 2023 19:19

Merge branch 'main' into feat/12698

ad77e83

change variable name

5a8363c

Merge branch 'main' into feat/12698

45c2706

roryabraham approved these changes Mar 22, 2023

View reviewed changes

roryabraham merged commit 359eb18 into Expensify:main Mar 23, 2023

OSBotify mentioned this pull request Mar 24, 2023

Deploy Checklist: New Expensify 2023-03-24 #16489

Closed

44 tasks

thienlnam mentioned this pull request Mar 29, 2023

LHN - Default offline avatar and lines instead of spinner are displayed when logging into account #16495

Closed

6 tasks

roryabraham mentioned this pull request Mar 30, 2023

[HOLD for payment 2023-05-22] [HOLD #14490] [$1000] Crash on IOURequestPage #16739

Closed

6 tasks

bernhardoj mentioned this pull request May 12, 2023

[HOLD for payment 2023-05-25] [$1000] Skeleton UI on 'Invite new member' page overlaps with 'Next' button #18813

Closed

6 tasks

bernhardoj mentioned this pull request May 24, 2023

[HOLD for payment 2023-06-05] [$1000] An overlapped skeleton UI in the payment details section #19442

Closed

6 tasks

manjesh-yadav mentioned this pull request Jun 16, 2023

[$500] Web - LHN - New workspace option disappear for few seconds when switching from offline to online #20198

Closed

6 tasks

chiragkcodes mentioned this pull request Oct 22, 2023

[HOLD for payment 2023-11-09] [$500] FAB - New workspace option disappears from FAB in offline mode #30138

Closed

6 tasks

hoangzinh reviewed Dec 18, 2023

View reviewed changes

hoangzinh mentioned this pull request Dec 19, 2023

[HOLD for payment 2024-01-09] [$500] Theme - Skeleton placeholder in LHN does not switch theme when device theme is switched #33163

Closed

6 tasks

neil-marcellini mentioned this pull request Dec 20, 2023

[$500] Chat-Infinite loading occurs if conversation ID is edited to 0 #32651

Closed

6 tasks

New skeleton loader for LHN #14456

New skeleton loader for LHN #14456

Conversation

bernhardoj commented Jan 21, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 21, 2023

shawnborton commented Jan 21, 2023

shawnborton commented Jan 21, 2023

bernhardoj commented Jan 22, 2023

aimane-chnaif commented Jan 22, 2023 • edited Loading

bernhardoj commented Jan 23, 2023

aimane-chnaif commented Jan 23, 2023

bernhardoj commented Jan 23, 2023

bernhardoj commented Jan 23, 2023

aimane-chnaif commented Jan 23, 2023

shawnborton commented Jan 23, 2023

bernhardoj commented Jan 23, 2023 • edited Loading

aimane-chnaif commented Jan 23, 2023

bernhardoj commented Jan 24, 2023

aimane-chnaif commented Jan 24, 2023

bernhardoj commented Jan 24, 2023

aimane-chnaif commented Mar 6, 2023

roryabraham left a comment

Choose a reason for hiding this comment

aimane-chnaif commented Mar 12, 2023

aimane-chnaif commented Mar 13, 2023 • edited Loading

bernhardoj commented Mar 14, 2023

aimane-chnaif commented Mar 21, 2023

bernhardoj commented Mar 21, 2023 • edited Loading

OSBotify commented Mar 23, 2023

bernhardoj commented Mar 23, 2023 • edited Loading

aimane-chnaif commented Mar 24, 2023

aimane-chnaif commented Mar 24, 2023

OSBotify commented Mar 24, 2023

OSBotify commented Mar 27, 2023

hoangzinh Dec 18, 2023

Choose a reason for hiding this comment

bernhardoj Dec 18, 2023

Choose a reason for hiding this comment

hoangzinh Dec 18, 2023

Choose a reason for hiding this comment

hoangzinh Dec 19, 2023

Choose a reason for hiding this comment

hoangzinh Dec 21, 2023

Choose a reason for hiding this comment

roryabraham Dec 28, 2023

Choose a reason for hiding this comment

bernhardoj commented Jan 21, 2023 •

edited

Loading

aimane-chnaif commented Jan 22, 2023 •

edited

Loading

bernhardoj commented Jan 23, 2023 •

edited

Loading

aimane-chnaif commented Mar 13, 2023 •

edited

Loading

bernhardoj commented Mar 21, 2023 •

edited

Loading

bernhardoj commented Mar 23, 2023 •

edited

Loading