Add launch screens. #4279

chrisbobbe · 2020-10-20T02:08:17Z

We've discussed a change in how we set the initial route when that becomes possible, i.e., when rehydration is complete.

In the proposed change (which should be possible even on our current version of React Navigation), we would just not render our navigation system's "Redux container" (a variant on React Navigation's own "App container", which we'll be using when #3804 is complete) while we're waiting for the Redux store to be rehydrated. (Then, when we do show it, we'll know exactly what to put for initialRouteName.)

That raises the question of what to show during that time. Currently, we show the LoadingScreen, but that's a component that's built to be embedded in our navigation system, and it probably wouldn't be great to duplicate the parts that aren't, and suit them to this purpose.

One attractive option is to extend the duration of the splash screen to cover the rehydration time. expo-splash-screen will let us do that (see its example for this pattern), and it also gives us lots of detailed instructions (ios, android) for setting up a splash/launch screen in the first place, even without using expo-splash-screen. That lets us fix #2756, though without anything fancy—just a solid brand-color background. But Apple's HIG document on the launch screen is pretty conservative about what should go in a launch screen, and a solid color seems quite likely to pass review.

For the record: I've found that Apple consistently prefers the term "launch screen"; I'm not sure about Android, but I think "splash screen" (what the Expo package name uses) is fine.

gnprice

Thanks @chrisbobbe ! This looks like a good set of changes to make.

I'm a bit puzzled by the last commit, the one that brings in the expo-splash-screen library. It seems like we're doing quite well already as of the commit before 🙂 , just using the platforms' respective features directly, with the library having served just as a guide for what features to use and what steps to take. The library adds some machinery and doesn't seem to make our own code any simpler.

Ah, rereading the PR description, I see the idea is to use expo-splash-screen's machinery to have the native launch screen persist until rehydration is done. I think it may be simpler to just control that in the standard React way, like we do most things in our app, as I suggested here in that thread.

ios/ZulipMobile.xcodeproj/project.pbxproj

ios/ZulipMobile/Assets.xcassets/LaunchScreen.imageset/Contents.json

gnprice · 2020-10-20T22:15:07Z

android/app/src/main/res/values/color.xml

@@ -2,4 +2,5 @@
 <resources>
    <color name="primaryColor">#ffffff</color>
    <color name="primaryColorDark">#d7ccc8</color>
+    <color name="splashscreen_background">#51c2af</color>


nit: follow surrounding capitalization convention, which is camelCase

(If using that Expo package, it apparently relies on the specific resource names. That's not a great API choice, and it's not real clearly stated in those setup instructions, but it's e.g. here in the implementation:
https://github.com/expo/expo/blob/227455e3d6f4bc9d05ca84109db5cbc03a56e342/packages/expo-splash-screen/android/src/main/java/expo/modules/splashscreen/NativeResourcesBasedSplashScreenViewProvider.kt#L32-L36
But we're not, so we don't.)

... Oh I see, using the Expo package is the next commit. 🙂

Through trial and error, I had noticed that it did depend on it literally being called "splashscreen_background" and thought it was odd, but I didn't locate it to the Expo package; I thought it might have been an Android requirement (still wouldn't have seemed quite right if that had ended up being the case).

My preference was actually something like "brand"—does that sound right? It would be nice to have something reusable on the Android native side for the brand color. And we may be able to bring it over the bridge with RN v0.63.0's fancy new PlatformColor API.

My preference was actually something like "brand"—does that sound right? It would be nice to have something reusable on the Android native side for the brand color. And we may be able to bring it over the bridge with RN v0.63.0's fancy new PlatformColor API.

Sure, SGTM!

android/app/src/main/res/values/styles.xml

android/app/src/main/AndroidManifest.xml

This one doesn't belong here -- we've made our own changes to it, which we don't want to get clobbered by running, e.g., `flow-typed update` or `yarn update-libdefs`.

Using `expo-splash-screen`'s Android-specific setup instructions [1], but with some important exceptions: - We don't (currently) intend to actually use `expo-splash-screen` [2], which means - Don't name a color "splashscreen_background". The API looks for that particular resource name. But we don't like its capitalization, which is inconsistent with the surrounding names [3]. - Don't use an image; we can add one later if we want. - Don't do anything marked "optional". - Don't go through a `splashscreen.xml` file; this extra indirection is unnecessary for us [4]. - Don't add an `android:theme="@style/AppTheme"` attribute to the `activity` element in our `AndroidManifest.xml`; it's redundant, with the same attribute already present in its parent `application` element. What's left is extremely small, and hard to describe, really, as "adding a splash screen". We're setting a background color, which will be what shows before our React elements have mounted and are showing. [1] https://github.com/expo/expo/tree/master/packages/expo-splash-screen#-configure-android [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5/near/1042794 [3] zulip#4279 (comment) [4] zulip#4279 (comment) Fixes: zulip#2756

chrisbobbe · 2020-10-21T03:15:17Z

Thanks for the review! We continued the conversation on CZO (around here), and decided to not use expo-splash-screen, so this revision cuts that out. I also responded to your comments above.

One thing to point out—the "add splash screen for Android" commit is really quite silly in how small it is, and it basically just looks like "set a root-level background color". I think Android doesn't have a strong, opinionated structure for making a launch/splash screen, like iOS does? 🤔

Anyway, with that background color being set, I notice something that reminds me of the (iOS-side) annoyance I mention in

95c57f2 ios: Set rootView.loadingView instead of rootView.backgroundColor.

I don't see anything odd when I rotate the device 90 degrees, but there's a kind of annoying appearance of that green background color when you open and close the keyboard. Try this reproduction, on Android:

In any narrow, tap the compose box, to type a message.
While the keyboard-appearance animation is in progress, watch as some solid brand-color space appears.
Tap away from the compose box, to make the keyboard disappear
Watch some solid brand-color space appear again (though I notice less of it here).

gnprice · 2020-10-21T21:08:43Z

docs/architecture/platform-versions.md

@@ -88,6 +88,9 @@ History:
  discovered that we'd been mistaken in thinking since 2018 that its
  WebView browser got updated independently; in fact it's pinned at a
  version a couple of years older than any other browser we support.
+* We dropped iOS 10 support in 2020-10. It was <0.3% of iOS users


nit: just "was 0.3%" -- in the message cited, that figure is rounded and the exact figure might have been anything from 0.25% to less than 0.35%

gnprice · 2020-10-21T21:15:31Z

android/app/src/main/res/values/color.xml

@@ -2,4 +2,5 @@
 <resources>
    <color name="primaryColor">#ffffff</color>
    <color name="primaryColorDark">#d7ccc8</color>
+    <color name="splashscreen_background">#51c2af</color>


My preference was actually something like "brand"—does that sound right? It would be nice to have something reusable on the Android native side for the brand color. And we may be able to bring it over the bridge with RN v0.63.0's fancy new PlatformColor API.

Sure, SGTM!

gnprice · 2020-10-21T23:37:15Z

I don't see anything odd when I rotate the device 90 degrees, but there's a kind of annoying appearance of that green background color when you open and close the keyboard.

In the chat thread we concluded that

the same problem exists in the Expo solution;
if we try the Android upstream recommendation for implementing a launch screen, it's foiled by RN -- there's a white flash in between the native launch screen and our LoadingScreen (i.e. between the native launch screen and the first React component tree);

and it seems like there isn't any readily-available solution for RN apps on Android that doesn't have one of those problems. So for Android, the best option is probably the status quo.

gnprice · 2020-10-21T23:39:25Z

LGTM other than that final Android commit and the one nit above (#4279 (comment)) -- after those please merge at will.

As of 2020-09-08, 99.7% of our iOS users are on iOS 11 or later [1]. As Greg says, """ With these figures, iOS 11.x joins iOS 10.x in being far into the range where if we learn things don't work or require effort to keep working, we'll cheerfully just drop support. """ Also, allow graceful degradation for iOS 12. As of Greg's report, versions <= 12 were 7.3% of iOS users, so graceful degradation at that level is fine. The particular feature we'll start using in an upcoming commit, which requires iOS 11, is "named colors" [2] [3] [4] [5]. [1] https://chat.zulip.org/#narrow/stream/48-mobile/topic/platform.20versions/near/1012197 [2] A pre-official release of Xcode 11 reportedly had a bug with this feature; see https://developer.apple.com/forums/thread/122661. The discussion there suggests that the bug was fixed in the "GM Seed 2" release for Xcode 11, and the build number for that release is the same as the build number for the official release (https://xcodereleases.com/). So we should be fine even if we use Xcode 11, which we don't (or I don't, anyway; I'm on 12). [3] https://developer.apple.com/library/archive/documentation/Xcode/Reference/xcode_ref-Asset_Catalog_Format/Named_Color.html [4] https://developer.apple.com/documentation/uikit/uicolor/2877380-colornamed?language=objc [5] https://devblog.xero.com/managing-ui-colours-with-ios-11-asset-catalogs-16500ba48205

We're about to add a "color set", which is a non-image asset that goes in this folder [1]. [1] https://help.apple.com/xcode/mac/current/#/dev10510b1f7

In a recent commit, we bumped the deployment target to 11.0, in order to handle a new feature: named colors [1] [2] [3] [4]. Now, add a color-set for the brand color, to be used in two places so far (in upcoming commits): - the iOS launch screen - the background color of `rootView.loadingView` in `AppDelegate.m` On iOS, it's easy to slightly mismatch the colors in the splash screen with the colors you use in the app, even if you give them equivalent definitions [5]. This is a move toward standardizing the brand color on iOS, to hopefully avoid that -- or, in any case, to reduce duplication of important color definitions. We don't yet send the color definition over the bridge and use it in our JavaScript, but we're hoping React Native v0.63's `PlatformColor` API [6] will let us do that easily, and we're hoping that will eliminate any issue. My eyes don't detect any difference between the HSL-defined color as it appears in the `LoadingScreen` background, and this RGB-defined color (HSL being apparently unavailable on the native side; I used an online converter) as it appears in the updated launch screen a few commits from now. [1] A pre-official release of Xcode 11 reportedly had a bug with this feature; see https://developer.apple.com/forums/thread/122661. The discussion there suggests that the bug was fixed in the "GM Seed 2" release for Xcode 11, and the build number for that release is the same as the build number for the official release (https://xcodereleases.com/). So we should be fine even if we use Xcode 11, which we don't (or I don't, anyway; I'm on 12). [2] https://developer.apple.com/library/archive/documentation/Xcode/Reference/xcode_ref-Asset_Catalog_Format/Named_Color.html [3] https://developer.apple.com/documentation/uikit/uicolor/2877380-colornamed?language=objc [4] https://devblog.xero.com/managing-ui-colours-with-ios-11-asset-catalogs-16500ba48205 [5] E.g., expo/expo#826 [6] https://reactnative.dev/docs/platformcolor

`rootView` is visible after the launch screen displays and before our React Native components are mounted and visible. It's therefore tempting to have `rootView`'s background mimic either the launch screen or the first thing that appears in the app (or both, if they're similar), so that it doesn't appear as a "flicker" in between. In particular, with the upcoming solid brand-colored launch screen, it's tempting to make `rootView.backgroundColor` also be the brand color. The reason this isn't great is that `rootView` isn't only visible at launch. Every time you rotate the device 90 degrees, it makes an appearance during the rotation animation [1]. It doesn't obscure anything, but it's annoying and distracting -- currently, it's explicitly white, and changing it to the brand color would probably make it even more jarring. So, don't set `rootView.backgroundColor` to anything. (The unwanted white areas in the rotation animation go away as a nice result of this.) Since we *do* want to show the brand color at launch, to avoid that flicker mentioned above, we can use RCTRootView's `loadingView` instead -- it's designed exactly for this. [1] See, e.g., the GIF at https://www.npmjs.com/package/react-native-root-view-background.

Use a "Storyboard" instead of an "XIB", as has apparently [1] been required since 2020-04-30. A change like this is part of the RN v0.62 -> v0.63 changes to the template app [2], corresponding to facebook/react-native@33b3a1a14. Since it's simple to do (especially after adding the "Brand" color set in a recent commit), make it be a solid brand-color screen instead of solid white. It should match the color of our `LoadingScreen`, which is often the first thing that displays when you open the app. The changes in this commit coincide with the iOS-specific instructions from `expo-launch-screen` for how to set up a launch screen [3], with some exceptions: - We don't actually go on to set up `expo-launch-screen`; we currently don't intend to [4]. - We don't do anything marked "optional". - We don't add an image to the launch screen; we can do that later if we want to. - We set the `Storyboard ID` to `LaunchScreenViewController`, not `SplashScreenViewController`. Apple regularly calls the thing a "launch screen", not a "splash screen". [1] https://developer.apple.com/news/?id=03042020b [2] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.3 [3] https://github.com/expo/expo/tree/master/packages/expo-splash-screen#manual-configuration [4] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5/near/1042794 [5] https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/launch-screen/

chrisbobbe · 2020-10-22T00:10:31Z

OK, sounds good. I just fixed that nit and removed that last commit for Android, and merged. Thanks again!

This is the RGBA representation of `hsl(222, 99%, 69%)`, which we've set as the BRAND_COLOR value in the JavaScript layer. I would put this in a code comment, but code comments aren't supported in JSON, which is unfortunate. We introduced this iOS colorset in 86843a1, to help us get to a world where it's easier to have the native iOS color definitions match those in the JavaScript layer. We haven't yet completed that work by using RN's new PlatformColor API (which is available to us now because we're using RN v0.63). But anyway, now that we've started using the new brand color in the JavaScript layer, we should change this one to match. Checking zulip#4279 again, we didn't add a counterpart native Android file that defines the brand color, and I didn't find an existing one, so I expect we don't need to make this kind of change for Android.

This is the RGBA representation of `hsl(222, 99%, 69%)`, which we've set as the BRAND_COLOR value in the JavaScript layer. I would put this in a code comment, but code comments aren't supported in JSON, which is unfortunate. We introduced this iOS colorset in 86843a1, to help us get to a world where it's easier to have the native iOS color definitions match those in the JavaScript layer. We haven't yet completed that work by using RN's new PlatformColor API (which is available to us now because we're using RN v0.63). But anyway, now that we've started using the new brand color in the JavaScript layer, we should change this one to match. Checking zulip#4279 again, we didn't add a counterpart native Android file that defines the brand color, and I didn't find an existing one, so I expect we don't need to make this kind of change for Android. (The color does appear in the native Android code directly, in a call to a `setColor` method for giving our notification icon a color.)

Empirically, this is no longer needed: we added it in c4fca9d to satisfy a peer-dependency constraint, but now I'm not getting a peer-dep warning if I remove it. See zulip#4279 and discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5/near/1042787 for some warnings from Greg about the design of expo-splash-screen, should we choose to actually use it one day.

This seems to be no longer needed: we added it in c4fca9d to satisfy a peer-dependency constraint, but now I'm not getting a peer-dep warning if I remove it. See zulip#4279 and discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5/near/1042787 for some warnings from Greg about the design of expo-splash-screen, should we choose to actually use it one day.

chrisbobbe added a-iOS a-Android labels Oct 20, 2020

chrisbobbe requested a review from gnprice October 20, 2020 02:08

chrisbobbe force-pushed the pr-launch-screen branch from d2483ce to b60c1fb Compare October 20, 2020 02:16

gnprice reviewed Oct 20, 2020

View reviewed changes

libdefs [nfc]: Move expo-application out of npm directory.

6f38de4

This one doesn't belong here -- we've made our own changes to it, which we don't want to get clobbered by running, e.g., `flow-typed update` or `yarn update-libdefs`.

chrisbobbe force-pushed the pr-launch-screen branch from b60c1fb to e32f089 Compare October 21, 2020 03:07

chrisbobbe changed the title ~~Add expo-splash-screen.~~ Add launch screens. Oct 21, 2020

gnprice reviewed Oct 21, 2020

View reviewed changes

chrisbobbe added 5 commits October 21, 2020 17:01

ios: Rename Images.xcassets to Assets.xcassets.

51551d3

We're about to add a "color set", which is a non-image asset that goes in this folder [1]. [1] https://help.apple.com/xcode/mac/current/#/dev10510b1f7

chrisbobbe force-pushed the pr-launch-screen branch from e32f089 to 78a62b2 Compare October 22, 2020 00:08

chrisbobbe merged commit 78a62b2 into zulip:master Oct 22, 2020

chrisbobbe mentioned this pull request Aug 3, 2021

deps: Remove expo-splash-screen. #4937

Closed

chrisbobbe deleted the pr-launch-screen branch November 5, 2021 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add launch screens. #4279

Add launch screens. #4279

chrisbobbe commented Oct 20, 2020 •

edited

Loading

gnprice left a comment

gnprice Oct 20, 2020

gnprice Oct 20, 2020

gnprice Oct 20, 2020

chrisbobbe Oct 21, 2020 •

edited

Loading

gnprice Oct 21, 2020

chrisbobbe commented Oct 21, 2020 •

edited

Loading

gnprice Oct 21, 2020

gnprice Oct 21, 2020

gnprice commented Oct 21, 2020

gnprice commented Oct 21, 2020

chrisbobbe commented Oct 22, 2020

Add launch screens. #4279

Add launch screens. #4279

Conversation

chrisbobbe commented Oct 20, 2020 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

gnprice Oct 20, 2020

Choose a reason for hiding this comment

gnprice Oct 20, 2020

Choose a reason for hiding this comment

gnprice Oct 20, 2020

Choose a reason for hiding this comment

chrisbobbe Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

gnprice Oct 21, 2020

Choose a reason for hiding this comment

chrisbobbe commented Oct 21, 2020 • edited Loading

gnprice Oct 21, 2020

Choose a reason for hiding this comment

gnprice Oct 21, 2020

Choose a reason for hiding this comment

gnprice commented Oct 21, 2020

gnprice commented Oct 21, 2020

chrisbobbe commented Oct 22, 2020

chrisbobbe commented Oct 20, 2020 •

edited

Loading

chrisbobbe Oct 21, 2020 •

edited

Loading

chrisbobbe commented Oct 21, 2020 •

edited

Loading