-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Port examples to use Create React App #669
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
Conversation
CLA is valid! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not the maintainer, but as the author of Create React App I wanted to share how I think this PR should look like.
To clarify what’s going on here, we are proposing a change to migrate this example to a build setup with Create React App so that React beginners have a cohesive experience running examples from top React libraries. You can read more about this effort here and more about Create React App here. I hope you’ll consider this PR, and if not, let us know what we could improve. Thanks!
@@ -3,26 +3,13 @@ React Intl Hello World Example | |||
|
|||
This is a very simple — yet runnable app — showing how to use React Intl to format message (string) that contains a placeholder, plural, and number. | |||
|
|||
## Running | |||
**This project template was built with [Create React App](https://github.com/facebookincubator/create-react-app).** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks too much like advertising here 😄
I think we should be more humble and just link to the user guide instead.
</head> | ||
<body> | ||
<div id="container"></div> | ||
<script src="https://cdn.polyfill.io/v1/polyfill.min.js?features=Intl.~locale.en"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to keep this script in index.html
.
work correctly both with client-side routing and a non-root public URL. | ||
Learn how to configure a non-root public URL by running `npm run build`. | ||
--> | ||
<title>React App</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the title
from original HTML file.
<head> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both favicon and the comment below can be completely removed, they are irrelevant to this project.
@@ -0,0 +1,8 @@ | |||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need for this file in this example, as it doesn’t have any tests.
@@ -0,0 +1,7 @@ | |||
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 841.9 595.3"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need for this file in this example.
"scripts": { | ||
"start": "react-scripts start", | ||
"build": "react-scripts build", | ||
"test": "react-scripts test --env=jsdom", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the test
command here because it is irrelevant to this example.
Perhaps eject
too.
@iswanj cool, thanks! I have this same example converted over to |
Thank you for merging! |
If this is ok with you, Then I'll port all examples to use Create React App
Reference to this issue