Skip to content

Add Splash Screen to Android/iOS. Hide when app data is available.#2472

Merged
Jag96 merged 4 commits intomainfrom
marcaaron-splash
Apr 20, 2021
Merged

Add Splash Screen to Android/iOS. Hide when app data is available.#2472
Jag96 merged 4 commits intomainfrom
marcaaron-splash

Conversation

@marcaaron
Copy link
Copy Markdown
Contributor

@marcaaron marcaaron commented Apr 19, 2021

Details

  • Adds a splash screen to iOS and Android apps
  • Delays rendering of content until the major app bottleneck completes (this is the reports being fully loaded for now)

Fixed Issues

Fixes #1950

Tests / QA Steps

iOS / Android

  1. Start from a signed out freshly installed app with no existing data
  2. Boot the app and verify a loading screen with the app logo appears
  3. Sign in
  4. Verify that the loading screen returns
  5. Verify that after some time the loading screen is removed and the main app becomes accessible

Web/Desktop/mWeb

  1. Test for regressions only as the feature should not be available on web or desktop platforms

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

2021-04-19_11-43-05

Android

2021-04-19_11-43-36

@marcaaron marcaaron self-assigned this Apr 19, 2021
@marcaaron marcaaron marked this pull request as ready for review April 19, 2021 21:54
@marcaaron marcaaron requested a review from a team as a code owner April 19, 2021 21:54
@MelvinBot MelvinBot requested review from nkuoch and removed request for a team April 19, 2021 21:54
@marcaaron
Copy link
Copy Markdown
Contributor Author

@Jag96 Gonna add you as a reviewer as well if that's cool

@marcaaron marcaaron requested a review from Jag96 April 19, 2021 21:55
Copy link
Copy Markdown
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just a couple of questions

// than updating props for each report and re-rendering had merge been used.
Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT_IOUS, reportIOUData);
Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, simplifiedReports);
Onyx.merge(ONYXKEYS.APP_DATA_LOADED, true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we're using merge and not set here? Is it to prevent a call to keyChanged?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason in particular merge() and set() will have the same behavior when the value we are setting is simple like this.

src/Expensify.js Outdated
initWithStoredValues: false,
},
appDataLoaded: {
key: ONYXKEYS.APP_DATA_LOADED,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we can use an existing key here instead of adding a new one? Or do we need to add a new one to prevent this from being subscribed to keys that change very often?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions if there's another key that would be better to use. But no, I wasn't trying to avoid unnecessary re-renders by giving this it's own key - it just didn't seem to fit in anywhere else.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, my thought was that since we're setting APP_DATA_LOADED in the same spot we're updating ONYXKEYS.COLLECTION.REPORT, we could just use ONYXKEYS.COLLECTION.REPORT as the trigger rather than add a new key.

The main reason I'd want to avoid adding a new key in this case is to prevent adding a generic definition for app_data_loaded. For example, this key will hide a splash screen when the report data finishes loading, but it has nothing to do with the personal data loading, or any other "app data" we decide to add later, so if somebody sees this generic key later on and uses it somewhere else, it could cause confusion.

If re-using the ONYXKEYS.COLLECTION.REPORT key doesn't make sense and we need to add a new key, maybe update this to be more specific, so something like CHAT_REPORTS_LOADED. Lmk if that makes sense!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could just use ONYXKEYS.COLLECTION.REPORT as the trigger rather than add a new key.

The collection key is a collection of other keys so we can't yet add arbitrary data to the object returned once all those other keys are combined together. If we could do that, we'd still have to create extra checks (e.g. by using shouldComponentUpdate()) to prevent re-rendering the entire app when something about any report changed when we are really only interested in one thing (whether the reports have loaded).

this key will hide a splash screen when the report data finishes loading, but it has nothing to do with the personal data loading, or any other "app data" we decide to add later, so if somebody sees this generic key later on and uses it somewhere else, it could cause confusion.

That's a great point. I think I will rename it to something specific like initialReportDataLoaded and also provide a clearer comment about when it should be true/false. This should help avoid the situation you're describing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'd still have to create extra checks (e.g. by using shouldComponentUpdate()) to prevent re-rendering the entire app when something about any report changed when we are really only interested in one thing (whether the reports have loaded)

Got it, that makes sense. Renaming the key then works for me!

Copy link
Copy Markdown
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jag96 Jag96 merged commit dd5adba into main Apr 20, 2021
@Jag96 Jag96 deleted the marcaaron-splash branch April 20, 2021 18:18
@OSBotify
Copy link
Copy Markdown
Contributor

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

@OSBotify
Copy link
Copy Markdown
Contributor

🚀 Deployed to staging 🚀

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

@OSBotify
Copy link
Copy Markdown
Contributor

OSBotify commented May 8, 2021

🚀 Deployed to production in version: 1.0.39-5🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display loading image until e.cash is usable

3 participants