-
Notifications
You must be signed in to change notification settings - Fork 985
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
Implement new splash screen in its static version #15058
Conversation
Jenkins BuildsClick to see older builds (106)
|
hey @briansztamfater are we sure it won't be animated soon? just to do not do double work, also |
Hey @flexsurfer we are unsure when / how will it be animated, but either way since Android 12 forces a new Splash Screen API that shows before the other one that we currently have and that also supports animations, it is necessary to do some changes on our implementation and migrate it. https://developer.android.com/develop/ui/views/launch/splash-screen |
f0628bc
to
983e4b5
Compare
72f217b
to
28e0199
Compare
28e0199
to
8e1b1ae
Compare
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.
i don't seeorg.devio.rn.splashscreen.SplashScreen
removed from the deps
@flexsurfer that package is part of |
8e1b1ae
to
9a8b66c
Compare
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.
If you are just changing Gradle/Node/Gem dependencies by changing data files like deps.json
there is no need to include me in the PR.
Include me if you are changing actual Nix derivations or scripts. Not just a JSON file.
@briansztamfater hi! Could you please rebase the PR and move it to E2E column if it is ready for testing? Thank you! |
@flexsurfer that is just a placeholder for now as we are undergoing a small polish to the status brand so it will have to change before release |
Done ✅ |
90% of end-end tests have passed
Failed tests (3)Click to expandClass TestActivityMultipleDevicePR:
Class TestDeeplinkOneDeviceNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (28)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestDeeplinkOneDeviceNewUI:
|
hey @briansztamfater thank you, just want to make sure, does that mean current splash screen will have same issues ? |
@briansztamfater thanx for the fixes. Please take a look at a few more issues: Still reproducing ISSUE 4 (Dark screen is shown after splash screen when reopening app) on Huawei P20 light, android 9 At the same time this issue has gone for Samsung Galaxy A52, Android 12 and Huawei P40 Lite EMUI 12, Android 10 telegram-cloud-document-2-5229232384934684612.mp4 |
ISSUE 5 Light stripe in the bottom of the splash screen when light mode is enabled on IOSTested on iPhone X, IOS 14.1; iPhone SE, IOS 16.1.1; iPhone 11 Pro Max, IOS 16.3.1 telegram-cloud-document-2-5226510342036663728.mp4 |
082caf8
to
f21a893
Compare
@pavloburykh I increased the delay on the splash screen which should fix the blank screen on Huawei P20 light with Android 9, also although I couldn't reproduce the other two issues, I think I found the problems and pushed a fix. Let me know how testing goes on those. |
thanx @briansztamfater ! ISSUE 4 and ISSUE 5 are fixed. ISSUE 6 is still reproducible, I think this issue can be addressed in separate PR in order not to block current one. Regarding increased delay: I should admit that increasing delay to current level makes splash screen displaying for too long time on Android devices (see video below) and I am not sure If this is an optimal solution for us. telegram-cloud-document-2-5228762141850347976.mp4If there is no other solution I guess we will have to put up with ISSUE 4 but decrease delay to some more optimal and acceptable level. |
18% of end-end tests have passed
Not executed tests (9)Failed tests (18)Click to expandClass TestDeeplinkOneDeviceNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (4)Click to expandClass TestDeeplinkOneDeviceNewUI:
Class TestCommunityOneDeviceMerged:
|
I totally agree regarding the delay, if it is okay with the previous value I can revert it to that delay value for the time being until we are able to apply the optimal fix which depends on this PR being merged in our navigation library wix/react-native-navigation#7586 . We should create an issue to track this. |
Thanx @briansztamfater ! Because I have just checked this build https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-230314-200310-04d1f9-pr15058-universal.apk once again and delay still seems to be too long, though it fixed ISSUE 4 for Samsung Galaxy A52, Android 12 and Huawei P40 Lite EMUI 12, Android 10 telegram-cloud-document-2-5228762141850348296.mp4 |
e1634ec
to
5528416
Compare
Based on your videos I reduced it to 3.2 seconds, still considerable amount but maybe acceptable for some devices. Either way is just an arbitrary value and is temporal until we do it the proper way, which would hide the splash when the first screen is loaded depending on the loading time. Also when we add animations this will feel smother, and we will have a Splash implementation with the proper native API on Android. |
I've just tested our production app from the stores and for me it is showing two Splash screens, the Android 12 splash screen which is currently empty and shown forcedly by the OS, and then the custom animated one that is shown immediately after. Hence that's why is important to use the proper native API for the Splash Screen, even more because it supports animations natively now. |
Issue 6 should be fixed in latest build also, I was able to reproduce in a custom emulator and not happening anymore on the latest build |
94% of end-end tests have passed
Failed tests (2)Click to expandClass TestDeeplinkOneDeviceNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (29)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestDeeplinkOneDeviceNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
@briansztamfater thank you for the fixes. PR is ready to be merged.
telegram-cloud-document-2-5228811229031574029.mp4 |
674b2d4
to
739d82c
Compare
Signed-off-by: Brian Sztamfater <[email protected]>
739d82c
to
74da82c
Compare
fixes #15011
Summary
Implement new splash screen in its static version as the animation is not defined yet
iOS
Android
Development notes
For Android, we had to migrate to new Splash Screen API, so the changes were not as simple as I expected. Keeping
react-native-lottie-splash-screen
library because maybe we will need it, but if not we have to remove it on the following PR when we add animations.Testing notes
Platforms
Steps to test
status: ready