Polish the new app screen #24737

cpojer · 2019-05-07T13:06:57Z

Summary

Continuation of #24687

Issue: Polish the "new app screen"
This is the pull request for the new intro screen proposal in react native as directed by @cpojer

This PR was created because the previous one could not be pushed to for some reason. I cleaned up a few small things and added the component as an example to RNTester so we can keep iterating. My plan is to land this, and then polish it and make it the default in a follow-up.

Changelog

[General][Added] - New Intro screen, Icons

Removed Lottie Integration
100% React Native 💥

Test Plan

npm run ios should build the template app similar to this:

cpojer · 2019-05-07T13:08:37Z

@eliperkins @elucaswork @gengjiawen do you have any current feedback before landing this? If not, please approve it. I can then land it, and if you like you can send more PRs to polish it.

@orta could you give this a spin and tell me whether it matches your mockup to the point where you feel comfortable shipping it?

orta · 2019-05-07T14:30:43Z

Yeah, this looks good to me.

I don't know if it's your plan, but I'd love it if the index.js from this was the file in the template in your app, so you see some examples on how to do styling, platform checking, ScrollViews etc. ( e.g. not just render() { <NewAppScreen/> } in user-land code)

Libraries/NewAppScreen/index.js

cpojer · 2019-05-07T14:58:01Z

@orta I'm trying to figure out what a good middle ground is for the App template. On one hand I don't want to increase the app size a lot or bloat the template but on the other I do want there to be more than a single React component that is inlined. Do you have any recommendation on a good balance?

orta · 2019-05-07T15:32:21Z

My opinion, based on the idea that this is also "sample code" for when you first see react native. Move anything too 'complex' into react-native which is privately exported (and not considered the API):

Making the index.js' header more like:

import {View, Text, StyleSheet, ImageBackground} from 'react-native';
import {Header, Section, LearnMoreLinks, colors} from 'react-native/components/new-app-screen';

Which would contain the components we see today:

Header
Section
Learn More Links
colors
An abstraction that combines the Fragment, and the two SafeAreaViews?
I'm 60/40 on moving ReloadInstructions and DebugInstructions into it too.

If this is the sample code to give you a feel for what your React Native app will look like, then it's good to show:

Using a ScrollView
Using a StyleSheet (and how it hooks up to the style prop)
Using a Text and understanding how the text is a child of it
(Ideally) Using a TouchableOpacity to show how to do something which interacts (from reading the current code, this looks tricky)

As these are kinda the primitives for making an app at all. Given that RN ships untranspiled, an editor should be able to jump to the 'hidden' components in RN to read.

luisfmsouza · 2019-05-07T15:49:52Z

Libraries/NewAppScreen/components/Colors.js

+  light: '#DAE1E7',
+  dark: '#444',
+  black: '#000',
+};


We are mixing color names (white and black), hierarchy pattern (primary) and color states (lighter, light, dark). Think would be better to use only one pattern to name colors here, it will help to understand the code.

Suggestion:

export default { bondiBlue: '#1292B4', white: '#FFF', whiteSmoke: '#F3F3F3', pattensBlue: '#DAE1E7', charcoal: '#444', black: '#000', };

I've used this site as a reference

eliperkins

LGTM. Sounds good to land this as is, and iterate from here. There's a couple of odd things in there, but I can submit other PRs to clean them up after if we'd like.

eliperkins · 2019-05-07T15:47:08Z

Libraries/NewAppScreen/components/Header.js

+    backgroundColor: Colors.lighter,
+  },
+  backgroundLogo: {
+    position: 'absolute',


I'm fine landing this in for now, but I definitely fear other folks learning RN and styling for the first time picking up on using absolute to style things.

Definitely agree with this, is position: absolute required here? I hardly use it.

eliperkins · 2019-05-07T15:47:25Z

Libraries/NewAppScreen/components/Header.js

+    opacity: 0.2,
+    alignItems: 'center',
+    justifyContent: 'center',
+    height: 540,


Do these heights need to be explicit? What does this look like in landscape or on tablets?

eliperkins · 2019-05-07T15:50:19Z

Libraries/NewAppScreen/index.js

+const App = () => {
+  return (
+    <Fragment>
+      <SafeAreaView style={styles.topSafeArea} />


I'm still a bit confused why we need two SafeAreaViews. My understanding is that the SafeAreaView with the children should handle both top and bottom safe area insets.

@eliperkins I used two safe areas because I wanted the top safe area with the color of the header and the bottom with the color of the screen background. There is another way to do this?

Ah, interesting! I hadn't thought about that use-case. This totally makes more sense why we would reach for multiple <SafeAreaView>s now.

One option could be to use ScrollView's contentInsetAdjustmentBehavior: https://facebook.github.io/react-native/docs/scrollview.html#contentinsetadjustmentbehavior

To do this, we would have to move the ScrollView up to be the root view (that is, outside of the SafeAreaView this is currently containing it) and this will let UIKit take over on adjusting the insets for the safe areas. Then, by setting the background of the ScrollView to be Colors.lighter, we could accomplish this all without SafeAreaView at all!

I put a Snack up of this here: https://snack.expo.io/@eliperkins/lonely-churros

The downside to this approach is that the end of our scroll view would then be the grey color as well. I think the trade off is better than over-prescribing <SafeAreaView>s!

This could also use Animated to change the color of the <ScrollView>'s backgroundColor while scrolling, but that seems a bit too complex for a sample app.

lucasbento · 2019-05-08T06:51:44Z

Libraries/NewAppScreen/components/Header.js

+    backgroundColor: Colors.lighter,
+  },
+  backgroundLogo: {
+    position: 'absolute',


Definitely agree with this, is position: absolute required here? I hardly use it.

lucasbento · 2019-05-08T06:57:38Z

Libraries/NewAppScreen/index.js

+  sectionTitle: {
+    fontSize: 24,
+    fontWeight: '600',
+    color: '#000',


Please use Colors.black.

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cpojer · 2019-05-08T13:47:27Z

Thanks for all your feedback. I'm going to land this so we have the initial version in the repo and we can keep iterating together.

@eliperkins @lucasbento: would you two be interested in sending follow-up PRs? I made an issue for all the remaining items, see #24760:

Fix up the styling and color names a bit more.

Split up the components the way @orta proposed so we'll have some good sample code in the App.js template.

Make it so that links tapped will send an open-in-browser command via Metro on the host instead of in the emulator. See https://github.com/react-native-community/cli/blob/885f2c28d4e8ed66e334fd7bd0174173b2a394e1/packages/cli/src/commands/server/middleware/openStackFrameInEditorMiddleware.js for how we are doing this in the cli/metro (this may need some changes to the CLI and Metro)

Add a "Follow us on Twitter" button 😄

Actually put this all in the template so it can be shipped as part of 0.61.

react-native-bot · 2019-05-08T22:00:06Z

This pull request was successfully merged by @cpojer in 6b393b2.

^{When will my fix make it into a release? | Upcoming Releases}

Summary: Related to #24760 and #24737 This simplifies some styling within the components used for the New App Screen to help advocate for best practices when styling with CSS-like styles in React Native, as well as using React Native's own unique components. There's a bit more detail in each commit. Let me know if you'd like me to pull that info out into the PR description here! [General] [Changed] - Polished up new app screen component styling Pull Request resolved: #24783 Differential Revision: D15284851 Pulled By: cpojer fbshipit-source-id: 954db00d39fc0082bbd4dc96afa7d38dfb7f67d5

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 7, 2019

cpojer mentioned this pull request May 7, 2019

New Intro Screen Proposal #24687

Closed

facebook-github-bot added RN Team p: Facebook Partner: Facebook Partner labels May 7, 2019

cpojer changed the title ~~Polish app screen~~ Polish the new app screen May 7, 2019

react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label May 7, 2019

gengjiawen approved these changes May 7, 2019

View reviewed changes

estevaolucas approved these changes May 7, 2019

View reviewed changes

Libraries/NewAppScreen/index.js Outdated Show resolved Hide resolved

luisfmsouza reviewed May 7, 2019

View reviewed changes

eliperkins approved these changes May 7, 2019

View reviewed changes

lucasbento reviewed May 8, 2019

View reviewed changes

therealadum and others added 14 commits May 8, 2019 14:38

initial welcome screen rewrite

7688258

Add Lottie animation

afc03c2

Improve styles

73428b4

Remove unused dependency

cd2ac36

Unlink unused dependency

828c09f

Improve safe area

4a0294f

Fix typo

5acf684

Update header to light version and fix some styles

2964ddd

Fix reload and debug instructions

3e83f89

added default logo

804c2f9

removed lottie and updated logos

f632873

removed lottie and updated package.json

4c6de62

added prettier rules

b3063c6

removed duplicate index module

d814c57

karanpratapsingh and others added 13 commits May 8, 2019 14:38

updated for cirlcle ci

90ec258

fix for node_its checks

4772db7

ahhhhh node_its test suite update formatting

e13a598

added @glauberfc changes

631e8d4

removed proptypes

87d7dab

minor changes in Header component

be706df

removed template files

befe3d1

fixed linting (I hope so) T_T;

cebfd94

trying to please linting gods

189dfd7

moved to Libraties/NewAppScreen

47dd6f5

added copyright headers

8842ee8

formatted with prettier

4205f18

Cleanup + Add to RNTester

efc40bc

cpojer force-pushed the polish-app-screen branch from 7f9c95f to efc40bc Compare May 8, 2019 13:41

facebook-github-bot approved these changes May 8, 2019

View reviewed changes

cpojer mentioned this pull request May 8, 2019

New App Screen #24760

Closed

5 tasks

facebook-github-bot closed this in 6b393b2 May 8, 2019

react-native-bot added the Merged This PR has been merged. label May 8, 2019

eliperkins mentioned this pull request May 9, 2019

Polish new app screen component styling #24783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish the new app screen #24737

Polish the new app screen #24737

cpojer commented May 7, 2019

cpojer commented May 7, 2019

orta commented May 7, 2019

cpojer commented May 7, 2019

orta commented May 7, 2019

luisfmsouza May 7, 2019

eliperkins left a comment

eliperkins May 7, 2019

lucasbento May 8, 2019

eliperkins May 7, 2019

eliperkins May 7, 2019

glauberfc May 7, 2019

eliperkins May 7, 2019

eliperkins May 7, 2019

lucasbento May 8, 2019

lucasbento May 8, 2019

facebook-github-bot left a comment

cpojer commented May 8, 2019

react-native-bot commented May 8, 2019

Polish the new app screen #24737

Polish the new app screen #24737

Conversation

cpojer commented May 7, 2019

Summary

Changelog

Test Plan

cpojer commented May 7, 2019

orta commented May 7, 2019

cpojer commented May 7, 2019

orta commented May 7, 2019

Choose a reason for hiding this comment

eliperkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

cpojer commented May 8, 2019

react-native-bot commented May 8, 2019