-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Conversation
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 [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
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? |
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 [email protected]
# or
yarn add [email protected]
# or
create-react-app [email protected] 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! |
@Timer not a bad shout! Let us see what others think 👍 |
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. |
Awesome! Me too.. |
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 |
@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:
They are just my thoughts though.. more than happy to discuss alternatives! |
@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. |
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. |
@joecritch @bvaughn I'd like to hear from you on this. |
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 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, 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. |
@joecritch thanks for the feedback 🙇
Good spot and suggestion;
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
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 |
Yep, I think that'd work well. Perhaps have
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. |
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;
} |
…ct-app into lukejacksonn-dark-theme
* Applies dark theme and adds link to reactjs.org * Just "learn React" * Tweaks
* Applies dark theme and adds link to reactjs.org * Just "learn React" * Tweaks
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
.App-header
background-color to#282c34
from reactjs.org.App-intro
into.App-header
and addedmin-height: 100vh
to header.App-link
CTA highlighted in#61daFb
which links to reactjs.org for reference.App-title
andTo get started,
text in.App-intro
for the sake of brevity.App-header
contents vertically and horizontally usingdisplay: flex
font-size: calc(10px + 2vmin)
on.App-header
💭 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 being100vh
. A solution to this would be setting the height of the header to90vh
and setting the background colour of.App
orbody
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!