-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Animate splash screen #17477
Animate splash screen #17477
Conversation
rerolling pullerbear, as I'm OOO on parental leave |
@marcochavezf Please 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] |
a9331ac
to
0b649e9
Compare
Hi @zoontek! Could you fix the conflict? |
0b649e9
to
7974701
Compare
@marcochavezf Done |
reviewing today |
@0xmiroslav Any news? |
iOS looks good. I am currently testing on various android devices including lower OS versions. will update soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android animation doesn't work at all for me. Tested on Android 13
NOTE: I merged main into this branch locally before testing
bug1.mov
@marcochavezf can you please add |
7974701
to
cb63f80
Compare
Done |
@zoontek is it ready for another review? |
@0xmiroslav Not yet. I published the updated animation (link) and removed |
6632c7d
to
151f4c3
Compare
@0xmiroslav I fixed the issue! It appears that the bootsplash module can be faster to hide the native splash screen than So I updated the logic, renamed I restarted the app a dozen of times, I cannot reproduce it anymore 🎉 |
ok, will review again. |
@0xmiroslav Done ✅ |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromescreen-20230518-165238.mp4Mobile Web - SafariRPReplay_Final1684454049.MP4DesktopiOSRPReplay_Final1684453577.MP4Androidscreen-20230518-164902.mp4 |
Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Hmmm I had the ad-hoc build from this branch installed and noticed that the ReportActionContextMenu wasn't working. Let's monitor and see if the bug also exists on staging once this is deployed |
This caused by this PR. I think it came from Expensify.js |
This is now fixed in #19244 and merged |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.17-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.17-5 🚀
|
@@ -73,11 +72,9 @@ const webpackConfig = ({envFile = '.env', platform = 'web'}) => ({ | |||
new HtmlWebpackPlugin({ | |||
template: 'web/index.html', | |||
filename: 'index.html', | |||
splashLogo: fs.readFileSync(path.resolve(__dirname, `../../assets/images/new-expensify${mapEnvToLogoSuffix(envFile)}.svg`), 'utf-8'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line caused a bug to arise, resulting in a regression in #19777. We should have verified that the storybook builds and runs successfully.
Details
This PR adds support for animated splash screen , instead of a simple fade out.
A few things:
react-native-reanimated
. As it's only scale and opacity, it's already 100% native animations (I also tried with reanimated, the behaviour / smoothness were identical)fallback
prop onNavigationContainer
, which was responsible for this ugly loading screen just after the splash screen hide. It appears their documentation say: "If you have a native splash screen, please use onReady instead of fallback prop.", so this is the way to go (Link)BaseAppTheme
android:statusBarColor
on Android, at start. Before, it was the color of the splash screen: it worked since we faded the native splash screen as a whole, making the system bars fade with it. It doesn't work with this new approach since the system bars cannot just switch from splash screen background color to app status bar color when removing the native splash screen (without fade). So instead, I immediately apply the app status bar color. It continues to "blink" in development on Android >= 12 (system thread splash screen uses transparent system bars, app thread splash screen does not), but it's not an issue on release as it's nearly immediate (check videos)navigationBarHeight
constant to be able to center the logo on window height (including system bars). It uses a logic 100% identical to react-native StatusBar module.Fixed Issues
$ #14151
PROPOSAL: #14151 (comment)
$ #18172
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
No changes (still a simple fade):
Desktop.Safari.mp4
Mobile Web - Chrome
No changes, similar to Web version.
Mobile Web - Safari
No changes, similar to Web version.
Desktop
No changes, similar to Web version.
iOS
iOS.simulator.mp4
Android
Android 13 (release, Pixel 4a, high-end device):
Pixel.4a.mp4
Android 10 (release, Xiaomi A2, low-end device):
Xiaomi.A2.mp4