Add Splash Screen to Android/iOS. Hide when app data is available. #2472
Conversation
|
@Jag96 Gonna add you as a reviewer as well if that's cool |
src/libs/actions/Report.js
Outdated
| @@ -315,6 +315,7 @@ function fetchChatReportsByIDs(chatList) { | |||
| // 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); | |||
There was a problem hiding this comment.
Any reason we're using merge and not set here? Is it to prevent a call to keyChanged?
There was a problem hiding this comment.
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
| @@ -119,4 +151,7 @@ export default withOnyx({ | |||
| key: ONYXKEYS.UPDATE_AVAILABLE, | |||
| initWithStoredValues: false, | |||
| }, | |||
| appDataLoaded: { | |||
| key: ONYXKEYS.APP_DATA_LOADED, | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
|
✋ 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 🚀
|
|
🚀 Deployed to production in version: 1.0.39-5🚀
|
Details
Fixed Issues
Fixes #1950
Tests / QA Steps
iOS / Android
Web/Desktop/mWeb
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android