Skip to content

Embed react-native-bootsplash lite module#7096

Merged
NikkiWines merged 17 commits intoExpensify:mainfrom
zoontek:embed-bootsplash-lite
Jan 20, 2022
Merged

Embed react-native-bootsplash lite module#7096
NikkiWines merged 17 commits intoExpensify:mainfrom
zoontek:embed-bootsplash-lite

Conversation

@zoontek
Copy link
Contributor

@zoontek zoontek commented Jan 8, 2022

Details

Following #5620

Has the issue still seems present, this PR embed a minimalistic version of the library, hybrid between v3 / v4.
iOS module is based on react-native-bootsplash v4 (meaning it uses RCTRootView.loadingView instead of a UIViewController), Android module is based on v3 (meaning it doesn't use the new SplashScreen API).

But, as opposed to the "full" module, this one is tailored to be as minimal as possible (for your needs):

  • The show method does not exists
  • No more promises or tasks queue
  • No more pause when the app is in background
  • No transitioning status on getVisibilityStatus
  • 10 secs after the RCTContentDidAppearNotification native event, hide is called no matter what, as a safety measure (👉 this value could be adjusted)

I also updated react-navigation + react-native-screens to the latest versions to prevent some issues (as react-native-screens is enabled by default since 3.0.0 and use native stacks, it's not safe to stay on a version that old, see the changelogs), and fixed the initial loading screens background colors.

Fixed Issues

#5620 (maybe? reproduction steps are not known)

Tests

  • Starts the app on iOS, check if the splash screen is visible then disappear
  • Retry by starting the app using a push notification
  • Retry by putting the app on background before the app init, then put the app on foreground again

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

iOS

Android

@roryabraham
Copy link
Contributor

Hi @zoontek, feel free to merge main into this branch to fix your merge conflicts and the failing tests.

@zoontek
Copy link
Contributor Author

zoontek commented Jan 11, 2022

@roryabraham Done ✅

@NikkiWines
Copy link
Contributor

Sorry for the delay @zoontek - I've been traveling this week and will try to get to this early next week!

@marcaaron
Copy link
Contributor

Took a quick look at @NikkiWines request. Seeing as we are updating react-navigation it would be great if we could do some quick testing on web, desktop, mobile web. The rest of these changes look pretty legit (though someone who is more familiar with native iOS and Android should maybe review).

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

I think you'll need to clear the conflicts and rerun pod install.

@zoontek
Copy link
Contributor Author

zoontek commented Jan 15, 2022

@Julesssss Done ✅

Julesssss
Julesssss previously approved these changes Jan 17, 2022
Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

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

minor style suggestions but overall looks good.

cc: @marcaaron or @roryabraham not sure if either of you wanted to give this one more once over before we :shipit:

@zoontek
Copy link
Contributor Author

zoontek commented Jan 18, 2022

@NikkiWines I pushed the changes.

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

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

👍 Looks good

@NikkiWines
Copy link
Contributor

@zoontek, I've been holding off on merging this because of the increase of discussion in the main issue but was wondering if you think we should just go ahead and see if this resolves the issue.

I'm also happy to hold off for now and see if any of the new voices in that conversation have any suggestions.

@zoontek
Copy link
Contributor Author

zoontek commented Jan 20, 2022

@NikkiWines As there is still no reproduction steps, I would recommend merging it and internal testing, yes 🙂

@NikkiWines NikkiWines merged commit 669c0d9 into Expensify:main Jan 20, 2022
@OSBotify
Copy link
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
Contributor

🚀 Deployed to staging by @NikkiWines in version: 1.1.31-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.1.32-0 🚀

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

@zoontek zoontek deleted the embed-bootsplash-lite branch February 16, 2022 09:20
@marcaaron marcaaron mentioned this pull request Nov 23, 2022
94 tasks
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.

6 participants