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.

Sign up for GitHub

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

Jump to bottom

Add README to generated project. #33

Merged
merged 4 commits into from
Jul 21, 2016
Merged

Add README to generated project. #33

merged 4 commits into from
Jul 21, 2016

Conversation

eanplatter
Copy link
Contributor

Fixes #29

@ghost ghost added the CLA Signed label Jul 20, 2016
eanplatter
eanplatter reviewed Jul 20, 2016
View reviewed changes
scripts/init.js
@@ -47,6 +47,7 @@ module.exports = function(hostPath, appName, verbose) {
);
});
copySync(path.join(selfPath, 'index.html'), path.join(hostPath, 'index.html'));
copySync(path.join(selfPath, 'README.md'), path.join(hostPath, 'README.md'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to get some feedback on where we wanted to keep a README for the project itself.

I assume we don't want to just copy over the README from create-react-app but rather a README that takes the user's app name and just lists out some basic getting started instructions.

I was going to put it in the ./src but we're copying all of those files into the ./src of the project, so I'd have to end up checking for it and omitting it.

Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's do a separate directory for host setup, and put another README there that is more practical (basically a howto document).

@eanplatter
Copy link
Contributor Author

Would it make sense to have all of the code that will be put into the user's project in one place?

So instead of the following being in the root of the library:

+-- injectedREADME.md
+-- index.html
+-- src
|   +-- App.js
|   +-- App.css
|   +-- ...

You have something more like this:

+-- hostFiles
|   +-- index.html
|   +-- README.me
|   +--src
|   |   +-- App.js
|   |   +-- App.css
|   |   +-- ...

That way when we're creating the user's files the method would be simpler, we can just forEach through the hostFiles and map it out into the user's project.

@vjeux
Copy link
Contributor

vjeux commented Jul 20, 2016

Sounds like a good idea to me

@eanplatter
Copy link
Contributor Author

ok, I'm working on a hostFiles setup right now, I'll send it up to this PR shortly!

@eanplatter eanplatter changed the title Add README to generated project. (WIP) Add README to generated project. Jul 20, 2016
eanplatter
eanplatter reviewed Jul 20, 2016
View reviewed changes
scripts/init.js
path.join(selfPath, 'templateFiles', filename),
path.join(hostPath, filename)
)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as with the src directory, all of the files within the templateFiles directory will be put into the root of the project.

@ghost ghost added CLA Signed labels Jul 20, 2016
@eanplatter
Copy link
Contributor Author

Hey @gaearon any idea why this travis test is failing on node 4 but not 6?

@ghost ghost added the CLA Signed label Jul 20, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 20, 2016

This is intentional, I’m fixing e2e script to check for failure occurring in #31.
The issue will be fixed by #36 but I want to make sure our e2e test catches such errors.

@eanplatter
Copy link
Contributor Author

Cool, should I hold off merging until this is fixed?

@gaearon
Copy link
Contributor

gaearon commented Jul 20, 2016

Please do, I’m almost finished there.

@eanplatter
Copy link
Contributor Author

No prob!

@gaearon
Copy link
Contributor

gaearon commented Jul 20, 2016

You’re good to go now, sorry for the interruption!

@eanplatter
Copy link
Contributor Author

Cool thanks, rebuilding now. I think this PR is to be reviewed

gaearon
gaearon reviewed Jul 20, 2016
View reviewed changes
package.json
@@ -13,7 +13,7 @@
"config",
"scripts",
"src",
"index.html"
"templateFiles"
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about moving src itself to templateFiles?
Also I’d prefer template to templateFiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I guess the template directory would be a good place for it, essentially a mirror of the default user app.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you go that way you’ll need to look for var relative = and adjust those paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kk, thanks for the tip. I'll hop on this later today after work.

@ghost ghost added the CLA Signed label Jul 20, 2016
@ghost ghost added the CLA Signed label Jul 20, 2016
@eanplatter
Copy link
Contributor Author

So now when a user runs npm run create-react-app myApp they get a file structure that looks like this:
screen shot 2016-07-20 at 8 19 26 pm

@eanplatter
Copy link
Contributor Author

@gaearon sorry this took so long! I think it's ready now.

@ghost ghost added the CLA Signed label Jul 21, 2016
gaearon
gaearon reviewed Jul 21, 2016
View reviewed changes
scripts/init.js
path.join(hostPath, 'src', filename)
);
});
copySync(path.join(selfPath, 'index.html'), path.join(hostPath, 'index.html'));
fs.readdirSync(path.join(selfPath, 'template')).forEach(function(filename) {
console.log('what even is this?', path.join(selfPath, 'template', filename))
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably don't want this log 😉

@gaearon gaearon merged commit a6edb52 into facebook:master Jul 21, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 21, 2016

Thanks!

@eanplatter
Copy link
Contributor Author

Thanks for putting up with me :)

@gaearon gaearon mentioned this pull request Jul 21, 2016
Merged
gaearon added a commit that referenced this pull request Jul 21, 2016
@gaearon
Fix local development paths
7482eea 
Local npm start and npm run build got broken by #33
gaearon added a commit that referenced this pull request Jul 21, 2016
@gaearon
Fix local development paths (#52)
e563b20 
Local npm start and npm run build got broken by #33
@eanplatter eanplatter mentioned this pull request Jul 22, 2016
Merged
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eanplatter @vjeux @gaearon