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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Applies new theme and adds docs link to template #3235

Merged
merged 5 commits into from
Jan 17, 2018

Conversation

lukejacksonn
Copy link
Contributor

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

screen shot 2017-10-03 at 22 40 52

😎 AFTER

screen shot 2017-10-03 at 21 28 39

💅 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).

image

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
Copy link

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
Copy link

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

@Timer
Copy link
Contributor

Timer commented Oct 4, 2017

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
Copy link

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
Copy link
Contributor Author

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

@Timer Timer added this to the 2.0.0 milestone Oct 4, 2017
@Timer
Copy link
Contributor

Timer commented Oct 4, 2017

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
Copy link
Contributor Author

Awesome! Me too.. 2.0.0 could be 🔥

@Hurtak
Copy link
Contributor

Hurtak commented Oct 8, 2017

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
Copy link
Contributor Author

lukejacksonn commented Oct 9, 2017

@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
Copy link
Contributor

Hurtak commented Oct 9, 2017

@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
Copy link
Contributor Author

lukejacksonn commented Oct 9, 2017

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
Copy link
Contributor

gaearon commented Oct 28, 2017

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

@joecritch
Copy link

joecritch commented Oct 29, 2017

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
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor

bvaughn commented Nov 1, 2017

@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;
 }

screen shot 2017-11-01 at 8 56 38 am

@gaearon gaearon changed the base branch from master to next January 17, 2018 19:16
@gaearon gaearon merged commit f6ad7fe into facebook:next Jan 17, 2018
akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
* Applies dark theme and adds link to reactjs.org

* Just "learn React"

* Tweaks
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
* Applies dark theme and adds link to reactjs.org

* Just "learn React"

* Tweaks
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants