New Intro Screen Proposal #24687

karanpratapsingh · 2019-05-02T14:00:50Z

Summary

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

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:

…gh/react-native into polish-app-screen

facebook-github-bot · 2019-05-02T14:03:36Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2019-05-02T14:22:56Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

cpojer

This is fantastic! Thank you so much to everyone who collaborated on this in such a short amount of time. In order to get this into merged, could you get rid of all the template files and extract the code for the intro screen into a single component? To test it, you can include it inside of RNTester within this repository as a new example view.

I recommend putting the component into a folder called “Libraries/NewAppScreen”.

eliperkins

I left some general feedback on the components here. I think it's super important to nail these components, as they'll be the first building blocks that users will see and learn from!

Great work on getting this going 😄

eliperkins · 2019-05-02T15:49:59Z

Libraries/Template/App.js

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


What is this SafeAreaView used for? My understanding is that SafeAreaViews should generally have children inside, so is this just a spacer? If so, what is it spacing that the second SafeAreaView is not?

This SafeAreaView is for change the top Safe area style. This was the only way that I learned to do this.

@eliperkins sure thing, what changes do you suggest?

Libraries/Template/App.js

eliperkins · 2019-05-02T15:57:38Z

Libraries/Template/Components/Header.js

+    backgroundColor: '#F3F3F3',
+  },
+  backgroundLogo: {
+    position: 'absolute',


This absolute positioning feels like a dangerous best-practice to recommend to first-time React Native users. I wonder if there's a way for us to tuck this away or hide this complexity a bit by having a cropped image instead?

I think that what we can do is put the logo above te text

@eliperkins what do you suggest?

I'm not sure about this one

Libraries/Template/Components/Header.js

eliperkins · 2019-05-02T15:59:19Z

Libraries/Template/Components/Header.js

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


Same with these explicit widths and heights. I'd like to see these use flexbox if possible!

Libraries/Template/Components/LinkList.js

eliperkins · 2019-05-02T16:02:09Z

Libraries/Template/Components/LinkList.js

+    paddingVertical: 8,
+  },
+  link: {
+    width: '40%',


Hm, I wonder how this will look on iPad and other tablets? What do you think about making this a maxWidth instead, given that we know the titles will generally be relatively short.

This was the result with maxWidth:

So I remake it with the Flex property. This was the result:

estevaolucas

@karanpratapsingh nice work.
I was not able to run locally but would be dupe if this template scales up and down properly with different font sizes (Settings app -> General -> Accessibility -> Larger Text)

Libraries/Template/Components/Header.js

estevaolucas · 2019-05-02T17:11:32Z

Libraries/Template/Components/Header.js

@@ -0,0 +1,41 @@
+import React from 'react'


I saw you've added flow config. Do you plan to add type definition for those files?

I'm not entirely familiar with flow sir so I don't have a plan for flow

base on what we have in the current App.js, and since Flow isn't required to run RN, I think it makes sense to not have Flow nor propTypes.

I agree w/ you @elucaswork

Libraries/Template/Components/LinkList.js

Libraries/Template/Components/Header.js

Libraries/Template/Components/Section.js

glauberfc · 2019-05-02T21:26:12Z

@cpojer what steps do I need to follow to help to solve the requested changes? I need fork the @karanpratapsingh repo or only update the fork of RN that I made? I need to pull some thing?

cpojer · 2019-05-02T21:35:34Z

You can take the code from this PR and send another pull request if Karan is ok with it :) ideally you can give Karan rights to push to your fork so you can keep working on it togetherY

…

________________________________ From: Glauber Castro <notifications@github.com> Sent: Thursday, May 2, 2019 10:29:09 PM To: facebook/react-native Cc: Christoph Nakazawa; Mention Subject: Re: [facebook/react-native] New Intro Screen Proposal (#24687) @cpojer<https://github.com/cpojer> what steps do I need to follow to help to solve the requested changes? I need fork the @karanpratapsingh<https://github.com/karanpratapsingh> repo or only update the fork of RN that I made? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#24687 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAADIKFNTS22RZCP5W3BOJTPTNMKLANCNFSM4HJ6UTQQ>.

glauberfc · 2019-05-02T21:46:28Z

@cpojer are you using some specific ESLint/Prettier rules on the old welcome screen? If yes, could you share theses configs?

glauberfc · 2019-05-03T01:17:43Z

@karanpratapsingh I made the most of changes requested. It is on my fork. We need to decide about 64bits image, the better position of the logo on the screen and the use of Flow.

karanpratapsingh · 2019-05-03T01:52:36Z

Sorry guys I took a nap, I'll be working along with @glauberfc for this, it'll be my pleasure :D

karanpratapsingh · 2019-05-04T09:30:44Z

@cpojer can you please explain the changes you require us to do before merge?

cpojer · 2019-05-04T11:12:27Z

Karan: remove all the template files and only commit the components needed to show the new app screen. We can then include it in RNTester and the app template and iterate on it.

…

________________________________ From: Karan Pratap Singh <notifications@github.com> Sent: Saturday, May 4, 2019 10:33:32 AM To: facebook/react-native Cc: Christoph Nakazawa; Mention Subject: Re: [facebook/react-native] New Intro Screen Proposal (#24687) @cpojer<https://github.com/cpojer> can you please explain the changes you require us to do before merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#24687 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAADIKF3KF77DOSY43KFWFLPTVJ6ZANCNFSM4HJ6UTQQ>.

karanpratapsingh · 2019-05-04T11:42:14Z

@cpojer I've removed the unnecessary files and only kept the component necessary for the new app screen...where should I put them next?

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Template/App.js

Libraries/Template/components/LearnMoreLinks.js

Libraries/Template/components/colors.js

cpojer · 2019-05-04T11:54:32Z

Nice! Please move it to “Libraries/NewAppScreen” and run “yarn lint --fix”. You can then check out the RNTester app in this repo and make an example that includes the component.

Please also add the copyright header that’s present in the other files :)

Thank you!

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Template/App.js

Libraries/Template/Components/Header.js

Libraries/Template/components/LearnMoreLinks.js

Libraries/Template/components/colors.js

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.
flow found some issues.

Libraries/NewAppScreen/App.js

Libraries/NewAppScreen/components/Colors.js

Libraries/NewAppScreen/components/Header.js

Libraries/NewAppScreen/components/LearnMoreLinks.js

Libraries/Template/App.js

Libraries/Template/Components/Header.js

Libraries/Template/Components/Section.js

Libraries/Template/components/LearnMoreLinks.js

karanpratapsingh · 2019-05-04T12:34:31Z

@cpojer I've moved it to Libraries/NewAppScreen, what is RNTester? how to use it? Sorry this is my first time contributing to this repo :D

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.
flow found some issues.

Libraries/NewAppScreen/App.js

analysis-bot · 2019-05-04T12:38:44Z

Libraries/NewAppScreen/components/Colors.js

+'use strict';
+
+export default {
+    primary: '#1292B4',


prettier/prettier: Delete ··

Libraries/NewAppScreen/components/Colors.js

Libraries/NewAppScreen/components/LearnMoreLinks.js

Libraries/NewAppScreen/components/Section.js

analysis-bot

Code analysis results:

flow found some issues.

analysis-bot · 2019-05-04T12:43:26Z

Libraries/NewAppScreen/components/Section.js

+import React from 'react';
+import {View, StyleSheet} from 'react-native';
+
+const Section = ({children}) => (


Missing type annotation for destructuring.

Maybe put it in Same file

react-native/RNTester/js/SectionListExample.js

Lines 46 to 51 in 38483d4

const renderSectionHeader = ({section}) => (

<View style={styles.header}>

<Text style={styles.headerText}>SECTION HEADER: {section.key}</Text>

<SeparatorComponent />

</View>

);

cpojer · 2019-05-04T12:44:12Z

Check out the RNTester folder, it contains lots of examples of RN components, and you can copy one of them to integrate the new app screen. You can run it on iOS or Android and the website has I instructions for it (sorry on mobile right now).

karanpratapsingh · 2019-05-04T12:45:14Z

@cpojer got it sir

gengjiawen

Can you highlight App.js, Cmd + R, Cmd + D ?

Update: I see the change already made in code, Can you update the picture in PR description.

karanpratapsingh · 2019-05-05T13:00:49Z

Hey, guys, I'll be away for a couple of days, feel free to take a fork and update this PR..sorry

cpojer · 2019-05-06T05:54:34Z

Thanks @karanpratapsingh!

Is there anybody who can take this over for now? :)

glauberfc · 2019-05-06T10:39:47Z

Thanks @karanpratapsingh!

Is there anybody who can take this over for now? :)

I can @cpojer

karanpratapsingh · 2019-05-06T10:47:42Z

Thanks @karanpratapsingh!
Is there anybody who can take this over for now? :)

I can @cpojer

Sure, @glauberfc please feel free to extend this...tho I'll be available for a short duration

cpojer · 2019-05-07T13:07:14Z

Created a follow-up PR here: #24737

Let's take it from there.

Summary: Continuation of #24687 > Issue: [Polish the "new app screen"](react-native-community/discussions-and-proposals#122) > 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. [General][Added] - New Intro screen, Icons Removed Lottie Integration 100% React Native 💥 Pull Request resolved: #24737 Differential Revision: D15259092 Pulled By: cpojer fbshipit-source-id: bc141fb1425cf354f29deffd907c37f83fd92c75

therealadum and others added 13 commits April 29, 2019 14:50

initial welcome screen rewrite

158d6ca

Add Lottie animation

b382f49

Improve styles

5e3b557

Remove unused dependency

30ddd53

Unlink unused dependency

29dc908

Improve safe area

308c525

Fix typo

2d3dd85

Update header to light version and fix some styles

ec4d8de

Fix reload and debug instructions

c80fe1a

Merge branch 'polish-app-screen' of https://github.com/karanpratapsin…

1ca8aa2

…gh/react-native into polish-app-screen

added default logo

88fae41

removed lottie and updated logos

764a279

removed lottie and updated package.json

bc448a3

react-native-bot added the Includes Changelog label May 2, 2019

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 2, 2019

karanpratapsingh added 5 commits May 2, 2019 19:55

added prettier rules

00ae0e3

removed duplicate index module

8367ed8

updated for cirlcle ci

ccccc62

fix for node_its checks

83561b1

ahhhhh node_its test suite update formatting

cb06d91

cpojer suggested changes May 2, 2019

View reviewed changes

eliperkins reviewed May 2, 2019

View reviewed changes

estevaolucas reviewed May 2, 2019

View reviewed changes

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

removed template files

2606679

analysis-bot suggested changes May 4, 2019

View reviewed changes

fixed linting (I hope so) T_T;

70b496a

analysis-bot suggested changes May 4, 2019

View reviewed changes

trying to please linting gods

23c6580

analysis-bot suggested changes May 4, 2019

View reviewed changes

moved to Libraties/NewAppScreen

8586ee6

added copyright headers

70fd9ca

analysis-bot suggested changes May 4, 2019

View reviewed changes

formatted with prettier

cc11be8

analysis-bot suggested changes May 4, 2019

View reviewed changes

gengjiawen suggested changes May 5, 2019

View reviewed changes

hramos removed the Includes Changelog label May 6, 2019

cpojer mentioned this pull request May 7, 2019

Polish the new app screen #24737

Closed

cpojer closed this May 7, 2019

eliperkins mentioned this pull request May 13, 2019

Replace new app template with new app screen #24805

Closed

	const renderSectionHeader = ({section}) => (
	<View style={styles.header}>
	<Text style={styles.headerText}>SECTION HEADER: {section.key}</Text>
	<SeparatorComponent />
	</View>
	);

New Intro Screen Proposal #24687

New Intro Screen Proposal #24687

Uh oh!

Conversation

karanpratapsingh commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

facebook-github-bot commented May 2, 2019

Uh oh!

facebook-github-bot commented May 2, 2019

Uh oh!

cpojer left a comment

Choose a reason for hiding this comment

Uh oh!

eliperkins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estevaolucas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estevaolucas May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glauberfc commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpojer commented May 2, 2019 via email

Uh oh!

glauberfc commented May 2, 2019

Uh oh!

glauberfc commented May 3, 2019

Uh oh!

karanpratapsingh commented May 3, 2019

Uh oh!

karanpratapsingh commented May 4, 2019

Uh oh!

cpojer commented May 4, 2019 via email

Uh oh!

karanpratapsingh commented May 4, 2019

Uh oh!

analysis-bot left a comment

karanpratapsingh commented May 2, 2019 •

edited

Loading

estevaolucas May 3, 2019 •

edited

Loading

glauberfc commented May 2, 2019 •

edited

Loading