Applies new theme and adds docs link to template

[lukejacksonn]

I was inspired by the new react docs, to readdress #1472 and give App.js some love and attention; changing as little as possible but embracing the new theme and adding a bit of responsiveness.

🙂 BEFORE

😎 AFTER

💅 CHANGES

• Updates .App-header background-color to #282c34 from reactjs.org
• Moves .App-intro into .App-header and added min-height: 100vh to header
• Adds .App-link CTA highlighted in #61daFb which links to reactjs.org for reference
• Removes .App-title and To get started, text in .App-intro for the sake of brevity
• Aligns .App-header contents vertically and horizontally using display: flex
• Optimises screen fit using font-size: calc(10px + 2vmin) on .App-header
• Increases the size of the rotating icon for dramatic effect

💭 THOUGHTS

One concern I have about this design is that if a first time user goes to App.js and enters content below .App-header then it won't be immediately visible due to the header being 100vh. A solution to this would be setting the height of the header to 90vh and setting the background colour of .App or body to #20232a (the darker grey in the theme).

The changes, although superficial, make the first creat-react-app experience a little more magical and a little more useful. It is in keeping with the react theme and portrays a truer representation of the products build quality and underlying technologies.

I'm aware this PR is massively subjective but let me know what you think. I am more than willing to discuss/make changes if you like the general direction!

[facebook-github-bot]

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]

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

[Timer]

Very neat! What if we went with the second option but put the "Learn About React" link down there so they don't think it's a visual quirk?

[react-scripts-dangerous]

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit ca35d9e) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.15-ca35d9e.0
# or
yarn add react-scripts-dangerous@1.0.15-ca35d9e.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.15-ca35d9e.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

[lukejacksonn]

@Timer not a bad shout! Let us see what others think 👍

[Timer]

I'll schedule this for 2.0.0 so there's a new face for a new major version. 😄

Excited to see feedback from others.

[lukejacksonn]

Awesome! Me too.. 2.0.0 could be 🔥

[Hurtak]

I like the visuals but I do not like how it is structured. Basically the header became page content but it is still called header. I would switch the tags from header to div or maybe even main, and rename the class too.

[lukejacksonn]

@Hurtak thanks for the feedback! In my previous PR #1472 I unwrapped the header contents into app. This time I decided not to unwrap for these reasons:

• I was trying to change as little as possible.
• Adding display: flex with align-items: center and justify-content: center to the app container really doesn't make the template very extensible.
• It is much more common place to find this kind of styling on an element like a header (especially as it is also propped open using min-height: 100vh).
• Allows people to add a main element (as you suggest) below the header without having any styles inflicted on them (as template styles are scoped to the header).

They are just my thoughts though.. more than happy to discuss alternatives!

[Hurtak]

@lukejacksonn I think you are overthinking it, most people will either just play with the template to see the live reload and other features of CRA, the rest will just delete the current templates/styles and add their own.

I would just try to have the simplest possible html and css that conveys the message (edit some file and see the changes) and have it structured by best practices.

[lukejacksonn]

Cool, thanks. I think we are both making assumptions about how people are using this page. Would be good to try get some actual data on this and eliminate speculation. That said, I stand by my justifications and believe the implementation adheres to best practices.

[gaearon]

@joecritch @bvaughn I'd like to hear from you on this.

[joecritch]

I like it. Feels spacious, and the spinning logo is more suited in this redesign, IMO. The scaling text is also a nice touch.

One minor bug — the logo overlaps the CTA at certain angles when animating, and makes it unclickable. Perhaps put pointer-events: none on the logo?

Edit: I share the concern about adding additional content below the fold. And the 90% height fix looks okay to me. But I would say that, with this design, App-header is no longer really a header, as it takes up the entire screen. That's just semantics though.

Also, after thinking about it (bear with me!), this redesign feels less like a "starting point" than the current one. That's not necessarily a bad thing, but just something to bear in mind.

[lukejacksonn]

@joecritch thanks for the feedback 🙇

The logo overlaps the CTA at certain angles when animating

Good spot and suggestion; pointer-events: none would do the trick.

I would say that, with this design, App-header is no longer really a header.

Agreed. I don't think there is any strict rule on header height but this is one reason why I explored an alternate design. The 90vh was an arbitrary value that was just less than 100vh. Thinking about it now, we could not prop it open at all and just add padding to the header. @Timer suggests putting some content (e.g. link to the docs) below, this would help justify having a header at all.

Feels less like a "starting point" than the current one.

How so, or rather, what would you add/remove/edit to make it feel more like a starting point? I feel whichever way you turn here, this is always going to be subjective to the intention of the developer (why did they run create-react-app in the first place) and you are not going to be able to please all of the people all of the time!

[joecritch]

@Timer suggests putting some content (e.g. link to the docs) below

Yep, I think that'd work well. Perhaps have .App itself as 100vh, rather than App-header Developers would be less likely to put something outside of it.

what would you add/remove/edit to make it feel more like a starting point?

Like you say, people will want to make all kinds of apps, so having something that is less of starting point makes it more obvious to delete what's in App.jsx and App.css. So its purpose has probably been improved.

This all gets my 👍 so far.

[bvaughn]

@joecritch @bvaughn I'd like to hear from you on this.

Hi 👋 I'm late to the party. No concerns or objections here though.

I like the fonts on reactjs.org too. Maybe we could steal them too? 😄

diff --git a/packages/react-scripts/template/src/index.css b/packages/react-scripts/template/src/index.css
index b4cc725..3f5f81f 100644
--- a/packages/react-scripts/template/src/index.css
+++ b/packages/react-scripts/template/src/index.css
@@ -1,5 +1,15 @@
 body {
   margin: 0;
   padding: 0;
-  font-family: sans-serif;
+  font-family: -apple-system, BlinkMacSystemFont,
+    "Segoe UI", "Roboto", "Oxygen", "Ubuntu", "Cantarell",
+    "Fira Sans", "Droid Sans", "Helvetica Neue",
+    sans-serif;
+  font-weight: 400;
+  font-style: normal;
+  -webkit-font-smoothing: antialiased;
+}
+
+code {
+  font-family: source-code-pro, Menlo, Monaco, Consolas, "Courier New", monospace;
 }