Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace new app template with new app screen #24805

Closed

Conversation

eliperkins
Copy link
Contributor

@eliperkins eliperkins commented May 10, 2019

Summary

This replaces the "new app screen" in the template with the new design from react-native-community/discussions-and-proposals#122

This uses components that are shipped as part of the react-native module, but not necessarily as proper components exported by the main react-native module. To use these, we use absolute imports to those components.

Related to #24760

Changelog

[General] [Changed] - Updated new app template design 馃挅

Test Plan

  1. Checkout this pull request (git fetch origin pull/ID/head:BRANCHNAME && git checkout BRANCHNAME, see also: https://help.github.com/en/articles/checking-out-pull-requests-locally).
  2. Change the react-native field in react-native/template/package.json to the absolute path of the react native repo, for example: "react-native": "file:///Users/username/react-native".
  3. Run npx @react-native-community/cli@next init TestApp --template file:///Users/username/react-native (If you don't have npx, run brew install npm).
  4. Note that a new app built with this template looks like the designs from react-native-community/discussions-and-proposals#122 !

Screen Shot 2019-05-10 at 12 29 25 PM

@facebook-github-bot facebook-github-bot added the CLA Signed label May 10, 2019
@eliperkins
Copy link
Contributor Author

@eliperkins eliperkins commented May 10, 2019

So this ends up clocking in at ~120 lines of code, which is over our ideal new app template of ~35-45 LoC.

One thought I have is to pull out the reload and debug instructions into the react-native/Libraries/NewAppScreen module itself, which should cut out ~25-30 LoC.

My second thought is to turn Section and it's subsequent Text usages into a more refined component, and tuck that into the NewAppScreen module as well. This seems to be where we introduce a lot of LoC. However, this is also the core usage of <View> and <Text>, so it'd be great to keep that around somehow.

I'm keen on some other ideas on how to reduce the lines of code here too!

@eliperkins eliperkins mentioned this pull request May 10, 2019
5 tasks
@orta
Copy link
Contributor

@orta orta commented May 11, 2019

I don't know where the number comes from but 30-40 LOC of the JSX in the render I think is totally fine 馃憤

Whether there value in having the two platform-y functions is a good Q - but I'm pretty happy with the rest of the code.

cpojer
cpojer approved these changes May 13, 2019
Copy link
Contributor

@cpojer cpojer left a comment

I agree we should trim it down. Let's tuck ReloadInstructions and DebugInstructions into the NewAppScreen and also put those sections into the LeanMoreLinks component (possibly named differently?). In terms of size that should trim it enough to be small but it will still have View/Text in it. Does that make sense?

Copy link
Contributor

@cpojer cpojer left a comment

Oops, meant to request changes :D

@eliperkins
Copy link
Contributor Author

@eliperkins eliperkins commented May 13, 2019

I don't know where the number comes from but 30-40 LOC of the JSX in the render I think is totally fine 馃憤

@orta I think this was an arbitrary number we pulled out that seems like it could be a good amount of code to leave as a first impression to RN. For some context, create-react-app's template clocks in at 26LoC, granted their styling is imported from a CSS module: https://github.com/facebook/create-react-app/blob/e2f43a06a60646f2db00ff0558acb6d2a3355a36/packages/react-scripts/template/src/App.js

I'll move stuff around like @cpojer recommended and we'll see how that feels!

eliperkins added 2 commits May 13, 2019
This will abstract over the platform checks and reduce the amount of hard coded copy we have in the initial app template.
@eliperkins eliperkins force-pushed the move-new-app-screen-to-template branch from a0f0708 to b266a5c Compare May 13, 2019
template/App.js Outdated Show resolved Hide resolved
template/App.js Outdated Show resolved Hide resolved
@eliperkins
Copy link
Contributor Author

@eliperkins eliperkins commented May 13, 2019

Let's tuck ReloadInstructions and DebugInstructions into the NewAppScreen

Done in b266a5c

and also put those sections into the LeanMoreLinks component (possibly named differently?).

Hmmmm, I'm not sure how this would work, without losing all the <Text>, in the app template. Currently (as of this PR), the structure is as follows:

<Fragment>
  <StatusBar/>
  <ScrollView>
    <Header />
    <View>{/* Sections with title & descriptions */}
      <Section>
        <Text />
        <Text />
      </Section>
      <Section>
        <Text />
        <Text>
          <ReloadInstructions />
        </Text>
      </Section>
      <Section>
        <Text />
        <Text>
          <DebugInstructions />
        </Text>
      </Section>
      <Section>
        <Text />
        <Text />
      </Section>
      <LearnMoreLinks />
    </View>
  </ScrollView>
</Fragment>;

In order to move <ReloadInstructions> and <DebugInstructions> into <LearnMoreLinks>, we'd likely have to move everything in the Sections with title & descriptions section into somewhere, leaving us with zero <Text> components.

@cpojer can you explain your thought process on how to move Reload and Debug into LearnMore for me?

@cpojer
Copy link
Contributor

@cpojer cpojer commented May 13, 2019

Right now it looks like this:

          <Section>
            <Text style={styles.sectionTitle}>Step One</Text>
            <Text style={styles.sectionDescription}>
              Edit <Text style={styles.highlight}>App.js</Text> to change this
              screen and then come back to see your edits.
            </Text>
          </Section>
          <Section>
            <Text style={styles.sectionTitle}>See Your Changes</Text>
            <Text style={styles.sectionDescription}>
              <ReloadInstructions />
            </Text>
          </Section>
          <Section>
            <Text style={styles.sectionTitle}>Debug</Text>
            <Text style={styles.sectionDescription}>
              <DebugInstructions />
            </Text>
          </Section>
          <Section>
            <Text style={styles.sectionTitle}>Learn More</Text>
            <Text style={styles.sectionDescription}>
              Read the docs on what to do once seen how to work in React Native.
            </Text>
          </Section>
          <LearnMoreLinks />

I think a bunch of this is superfluous. We only need a single <Text> in the template. Here is my suggestion:

          <Section>
            <Text style={styles.sectionTitle}>Step One</Text>
            <Text style={styles.sectionDescription}>
              Edit <Text style={styles.highlight}>App.js</Text> to change this
              screen and then come back to see your edits.
            </Text>
          </Section>
          <LearnMoreLinks /> {/* Everything removed compared to above is inside this component know */}

It still uses the same type of components as before at least once and is much much shorter in App.js.

eliperkins added 2 commits May 13, 2019
This component wasn鈥檛 really helpful to us, as it only applied a style that we could apply in render anyway.
This reduces duplicity in the new app screen, and makes it easier to understand what comes from this module.
@eliperkins eliperkins force-pushed the move-new-app-screen-to-template branch from b2f18f2 to 2bc2b1e Compare May 13, 2019
Colors,
DebugInstructions,
ReloadInstructions,
} from 'react-native/Libraries/NewAppScreen';
Copy link
Contributor

@thymikee thymikee May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this definitely needs an annotation that importing stuff from react-native/Libraries is only for demonstration purposes and one should never do it in a real app because it may break.

cpojer
cpojer approved these changes May 14, 2019
Copy link
Contributor

@cpojer cpojer left a comment

Fantastic! Thank you so much for the hard work on this. I'm going to land this version but we can update the comment in a follow-up.

@thymikee instead of a comment, what do you think of moving the index file of NewAppScript to react-native/NewAppScreen? That way it should be clear not to reach into libraries as it special cases this file only.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

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

@thymikee
Copy link
Contributor

@thymikee thymikee commented May 14, 2019

@cpojer makes sense

@react-native-bot
Copy link
Collaborator

@react-native-bot react-native-bot commented May 14, 2019

This pull request was successfully merged by @eliperkins in fe88e9e.

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

@react-native-bot react-native-bot added the Merged label May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants