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

Improve npm run eject description #363

Closed
wants to merge 106 commits into from
Closed

Improve npm run eject description #363

wants to merge 106 commits into from

Conversation

btnwtn
Copy link
Contributor

@btnwtn btnwtn commented Aug 4, 2016

I feel as if the description for npm run eject is incredibly vague. eject does more than just removing the tool and that should be explained to the user.

I feel as if the description for `npm run eject` is incredibly vague. `eject` does more than just removing the tool and that should be explained to the user.
@ghost
Copy link

ghost commented Aug 4, 2016

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 - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost ghost added the CLA Signed label Aug 4, 2016
@ghost
Copy link

ghost commented Aug 4, 2016

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

@gaearon
Copy link
Contributor

gaearon commented Aug 4, 2016

Can you show the screenshot?

If this doesn't look very well we should just reformat the whole message.

We can use chalk inversion to highlight commands, and write small descriptions below.

@btnwtn
Copy link
Contributor Author

btnwtn commented Aug 5, 2016

@gaearon screenshots supplied. Looks OK.

Improved description

Default description

Sorry for the massive images!

@ghost ghost added the CLA Signed label Aug 5, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

I don’t like that npm run eject prints multiple lines, inconsistent with earlier commands.
Can you please split list into separate sections with headers?

I was thinking something like

npm start

  Starts...

npm run build

  ...

npm test

  ...

npm run eject

  ...

We suggest that you ... 

Where all commands (npm start etc) are wrapped in chalk.cyan() (see scripts/build.js for example of this) so that they appear highlighted.

What do you think?

@btnwtn
Copy link
Contributor Author

btnwtn commented Sep 3, 2016

@gaearon sounds good to me, I'll have some time Monday to implement this. 👍

@ghost ghost added the CLA Signed label Sep 3, 2016
@gaearon gaearon added this to the 1.0.0 milestone Sep 3, 2016
@ghost ghost added the CLA Signed label Sep 3, 2016
@btnwtn
Copy link
Contributor Author

btnwtn commented Sep 10, 2016

@gaearon I've committed the changes that add better formatting and highlighting. Unfortunately, I've generated a node-debug log that I can't seem to get rid of 😦

@ghost ghost added the CLA Signed label Sep 10, 2016
@fson
Copy link
Contributor

fson commented Sep 11, 2016

@btnwtn You can get rid of the npm-debug.log file with git rm:

git rm npm-debug.log
git commit -m "Remove npm-debug.log"
git push origin master  # Push to your GitHub remote

Alternatively you can also remove it directly from the GitHub web interface, if you go to https://github.com/btnwtn/create-react-app/blob/master/npm-debug.log and click the thrash can icon.

@ghost ghost added the CLA Signed label Sep 11, 2016
console.log(' Bundles the app into static files for production.');
console.log();
console.log(chalk.cyan('npm run eject'));
console.log(' Removes this tool and copies build dependencies, configs, and scripts into the app directory.')
Copy link
Contributor

Choose a reason for hiding this comment

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

screen shot 2016-09-11 at 16 51 42

The new message looks good to me. But let's wrap this line at 80 characters, so it doesn't break with the "standard" terminal window size.

@btnwtn
Copy link
Contributor Author

btnwtn commented Sep 11, 2016

Deleted npm-debug.log and fit npm script descriptions to 80 characters. Looks good to me 😄

@fson
Copy link
Contributor

fson commented Sep 11, 2016

Thanks @btnwtn! Can you also merge create-react-app master branch into yours to resolve the conflicts. Happy to merge as soon as that's done.

@ghost ghost added the CLA Signed label Sep 11, 2016
@btnwtn
Copy link
Contributor Author

btnwtn commented Sep 11, 2016

Having issues with git... git pull says everything is up-to-date. I tried rebasing the commit(?) but that hasn't done anything. @fson what should I do?

Can't figure this out :\

dceddia and others added 26 commits September 11, 2016 11:02
With the HTTPS env var set 'true', the dev server will serve over HTTPS.
* Clarify proxy instructions in generated app README

* Add backticks to format text/html as code
For the 0.4.0 CHANGELOG entry, the npm install command should be:
```
npm install --save-dev --save-exact react-scripts@0.4.0
```

Not:
```
npm install --save-dev --save-exact react-scripts@0.3.1
```
* Change http-proxy-middleware logLevel from silent to error

* provide onError handler for httpProxyMiddleware
* Adds JSX extension support

* PR changes

* Add testRegex

* Add note about not recommending JSX, link to issue
…dleware. (#588)

* Change http-proxy-middleware logLevel from silent to error

* provide onError handler for httpProxyMiddleware

* Send proper error reponse upon proxy error.
* Warn about unsupported Node.js versions

Add the `engines` field to package.json so users of old Node.js versions
will at least get a warning when trying to install create-react-app or
react-scripts, e.g.:

  npm WARN engine create-react-app@0.3.0: wanted: {"node":">=6"} (current: {"node":"4.2.3","npm":"2.14.7"})

* Remove duplicated field and extra whitespace

* Change the engine version back to 4
The regexes in the Jest `moduleNameMapper` configs were a bit too strict,
causing them to not pick up files with special characters like `@` in the
file path. Change them to match anything with the correct file extension.
…with docker node (#620)

* Specify cache directory for babel loader

* Use `react-scripts` for folder name instead
Resolves an issue that can occur in certain situations that causes a build error when user has CDed into an incorrectly cased directory.
@btnwtn
Copy link
Contributor Author

btnwtn commented Sep 11, 2016

Oh for fucks sake. I'm going to close this, re-fork, and open a new pull request. @fson

@btnwtn btnwtn closed this Sep 11, 2016
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.