Skip to content
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

Display loading image until e.cash is usable #1950

Closed
sakluger opened this issue Mar 19, 2021 · 18 comments · Fixed by #2472
Closed

Display loading image until e.cash is usable #1950

sakluger opened this issue Mar 19, 2021 · 18 comments · Fixed by #2472
Assignees
Labels
Reviewing Has a PR in review

Comments

@sakluger
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding!


Platform - version: All, with a focus on mWeb and mobile apps.

Action Performed (reproducible steps):

  • Open the expensify.cash app on Android app, iOS app, or mobile web.
  • Wait until all the content loads.

Expected Result:

  • The app should show a loading screen until all the content is ready to display. We'd like the loading screen to show our Expensify.Cash icon and then immediately show the fully rendered content.

Actual Result:

  • The app loads in a bunch of steps, including several blank screens with slightly different shades.

Notes/Photos/Videos:
Startup on Android goes through several distinct phases:
a. First it opens to a pure white screen with a medium gray header
b. Then it goes to a dark gray screen with that medium gray header
c. Then dark gray with no header
d. Then full white with no header
e. Then the content appears
f. Then the header appears
g. Then the compose box appears

When we show all these different screens before rendering the content, it makes the app look unpolished and like it's not working correctly.

NOTE - We will focus on the speed of loading the app in separate issues since that will likely take work and time

Screenshots from current loading on app on Android

1
1-db

2
2-db

3
3-db

4
4-db

5
5 db

6
6-db

7
7db

@sakluger
Copy link
Contributor Author

@barun1997
Copy link
Contributor

barun1997 commented Mar 20, 2021

Hi,

I would love to work on this issue!

My proposal:

  1. Create a loading screen with Expensify.Cash icon centered in the view.
  2. Besides the absence of initial splash screen, the current screen goes blank when the following methods fetch in AuthScreens.js: PersonalDetails.fetch();
    PersonalDetails.fetchTimezone();
    getBetas();
    fetchAllReports(true, true) as well as when Onyx is migrated and when initialURL is resolved in NavigationRoot.js.

     Add a loading value in Onyx (or if already available use those loading states) -- for each of these data.

  1. Show the loading screen when the loading is true for all loading states, and render the content only when they are all false.

Please let me know if there are any suggestions for my solution!

@SameeraMadushan
Copy link
Contributor

SameeraMadushan commented Mar 20, 2021

Hi @sakluger,

Here is my proposal for this task.

Proposal

iOS and Android

  • I will use react-native-splash-screen for both android and iOS
  • This package will be used to get the animation from the native level. (When the app opens it will come with the splash icon)
  • With this package, we have to call SplashScreen.hide(); in the initial component.
  • For that, I will create src/libs/splash/native.index.js to implement native call. index.js will return false for other platforms.
  • After the native implementation, I will move to JS implementation.
  • We have the component loading hierarchy as follows.
App -> Expensify -> NavigationRoot -> AppNavigator -- // from here we have two paths

AuthScreens  -- // Data fetching is happening here
PublicScreens -- // login/signup and other screens
  • Likewise, Data fetching will be happening inside AuthScreens component.
  • Therefore I will add JS loading view inside AuthScreens.js. Because cannot add the loading screen to NavigationRoot then it won't render the AuthScreens component to fetch the data.
  • I look for the availability of the following outputs with ONYX
NameValuePair.get(CONST.NVP.PRIORITY_MODE, ONYXKEYS.PRIORITY_MODE, 'default');
PersonalDetails.fetch();
PersonalDetails.fetchTimezone();
getBetas();
fetchAllReports(true, true);
fetchCountryCodeByRequestIP();
  • Then I will add the custom loading view against AuthScreen's RootStack.Navigator with ternary and show hide the loading according to the data availability.

Web, Mobile web, and Desktop

  • For these platforms, only the JS part will be applied.

Status bar colors can be modified according to the requirement.

Thanks.

@mallenexpensify
Copy link
Contributor

@marcaaron assigned you to review proposals since you added the External tag here https://github.com/Expensify/Expensify/issues/155376#issuecomment-788204794

@marcaaron
Copy link
Contributor

Add a loading value in Onyx (or if already available use those loading states) -- for each of these data. Show the loading screen when the loading is true for all loading states, and render the content only when they are all false.

@barun1997 This plan seems reasonable, but I think incomplete.

This package will be used to get the animation from the native level. (When the app opens it will come with the splash icon)

@SameeraMadushan I'm curious to hear more about why this package is necessary.

index.js will return false for other platforms

Should we have the same effect on web? I think maybe, yes, since we may see issues on mobile web platforms where initial data fetching takes time to happen and the page is not yet interactive.

I look for the availability of the following outputs with ONYX

One thing to watch out for with this approach is that we will want to avoid any unnecessary re-renders at low levels when listening for changes to something like a loading property.

Separately, to address both proposals, I'm not entirely sure that the rendering issues are caused only by API calls.

I think a good baseline definition of "usable" would be...

  • All elements have loaded (that includes the user avatars)
  • The app is entirely interactive once content is displayed (tapping or scrolling works immediately)

@SameeraMadushan
Copy link
Contributor

@SameeraMadushan I'm curious to hear more about why this package is necessary.

@marcaaron splash screen package used only for mobile. It improves the user experience. Now when you open the app, the first thing you will see the white screen. With this package, instantly you will see the splash icon. ex: Twitter app

Should we have the same effect on web?

To have the same effect on the web, I'll only use the JS implementation.
The below diagram will explain the flow.
Screenshot 2021-03-25 at 01 24 00

One thing to watch out for with this approach is that we will want to avoid any unnecessary re-renders at low levels when listening for changes to something like a loading property.

This is true. We should avoid re-renders when checking those conditions. What I'm suggesting is not rendering the UI until all the necessary data is loaded.

I think this slowness occurs due to the API calls. You can clearly see the difference by,
Opening the app while you are logged in vs opening the app while you are logged out.

@marcaaron
Copy link
Contributor

marcaaron commented Mar 24, 2021

splash screen package used only for mobile. It improves the user experience

👍

You can clearly see the difference by, Opening the app while you are logged in vs opening the app while you are logged out.

That doesn't really tell the whole story. We load minimal UI elements when we are logged out so it's no surprise the initial render is quick. I have tested the proposed solution to delay rendering until after the APIs are complete with a production account that has many chats and users. To be sure, it helps some, but sorry does not solve this issue completely.

@kaushiktd
Copy link
Contributor

Hi,

This is Kaushik here. I would like to put a technical and pretty self explanatory solution for this. It is really simple,

  1. add a loader until data is load.

@mallenexpensify
Copy link
Contributor

Indirectly related, dropping in for reference https://expensify.slack.com/archives/C01GTK53T8Q/p1617740926401400
There should be a GH created for the above soon

@marcaaron
Copy link
Contributor

Hmm aren't these pretty much discussing the same issue? This issue feels like it has stalled a bit. Should we maybe come up with a more unified and complete plan, get design feedback, put together a mini doc or proposal and then create the issue?

@quinthar
Copy link
Contributor

quinthar commented Apr 7, 2021 via email

@mallenexpensify
Copy link
Contributor

For #1 above, I'm torn between a basic-ish loading animated issues that accomplishes what we want for an MVP (I do agree we should design eyes on this) or a "mini doc or proposal" I'm leaning towards the MVP then readdressing later, if needed. This issue would be specially for #1 and I can create a new issue for #2 and we can separately discuss that there.

@marcaaron what do you think about the above for a plan?

@marcaaron
Copy link
Contributor

Hmm. Not sure my previous comment about a mini doc is relevant anymore if these are separate issues. So it sounds like we need to:

  1. Figure out a plan to unblock this issue and do something about it (not sure what tbh)
  2. Create a new issue for the conversation you linked here earlier today.

@mallenexpensify
Copy link
Contributor

I think once we have feedback from the design team and the loading image ready, we can post to Upwork.
One thing I'm unsure about is if there will be significant differences between the platforms? I hope not since Desktop and web should load quickly when on stable internet.

@Expensify/design can you review the above, let us know if you have any concerns then propose the best option for an animated image? (not the finished product if you think it'd be helpful to discuss more first).

@shawnborton
Copy link
Contributor

@michelle-thompson already posted a mockup here, though it should be added to the original comment of this issue as well. The mockup she made uses the circular logo, and that asset already exists in E.cash (you can find it on the Sign In page).

@quinthar
Copy link
Contributor

quinthar commented Apr 7, 2021 via email

@shawnborton
Copy link
Contributor

For the sync indicator, perhaps one thing we could do is add some kind of loading spinner that circles around your profile picture, or replace the green dot with some kind of loading spinner/animation/etc.

@mallenexpensify
Copy link
Contributor

Let's not get into details for #2, the sync indicator in this issue, I created a fresssssh GH for that here https://github.com/Expensify/Expensify/issues/159767

Here is the image Shawn referenced above in case a contributor works on this and doesn't have access to that repo
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewing Has a PR in review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants