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

Beaufity output of eject.js script #769

Merged
merged 18 commits into from
Sep 30, 2016
Merged

Conversation

azakordonets
Copy link
Contributor

Solution for issue ##750

Since in build.js we already use chalk library, i decided to reuse it in eject.js file. Operations are marked with green, file names and path's are marked with yellow and are underlined. Screenshots are attached .
screenshot 2016-09-26 20 53 54
screenshot 2016-09-26 20 54 03
screenshot 2016-09-26 20 54 11

@ghost
Copy link

ghost commented Sep 26, 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!

@gaearon
Copy link
Contributor

gaearon commented Sep 26, 2016

I would prefer we keep the existing palette. We mostly use cyan and the default (no) color, can we do the same here? We also don’t use underlining everywhere else.

@@ -78,15 +81,15 @@ prompt(
var appPackage = require(path.join(appPath, 'package.json'));

var ownPackageName = ownPackage.name;
console.log('Removing dependency: ' + ownPackageName);
console.log('Removing dependency: ' + yellowUnderline(ownPackageName));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should have the same indentation as “adding a dependency” but they both need to be grouped under “Updating package.json

delete appPackage.devDependencies[ownPackageName];

Object.keys(ownPackage.dependencies).forEach(function (key) {
// For some reason optionalDependencies end up in dependencies after install
if (ownPackage.optionalDependencies[key]) {
return;
}
console.log('Adding dependency: ' + key);
console.log(green('\tAdding dependency: ') + yellowUnderline(key));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s use two-space indentation like in other commands.

@@ -62,7 +65,7 @@ prompt(
fs.mkdirSync(path.join(appPath, 'scripts'));

files.forEach(function(file) {
console.log('Copying ' + file + ' to ' + appPath);
console.log(green('Copying ') + yellowUnderline(file) + ' to ' + yellowUnderline(appPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s indent these and group them under “Copying files...”

@azakordonets
Copy link
Contributor Author

azakordonets commented Sep 27, 2016

Done, done and done. Attaching screenshots to show how it looks right now.
screenshot 2016-09-27 08 39 33
screenshot 2016-09-27 08 37 26
screenshot 2016-09-27 08 37 34

@ghost ghost added the CLA Signed label Sep 27, 2016
@ghost
Copy link

ghost commented Sep 27, 2016

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

@@ -76,21 +81,21 @@ prompt(

var ownPackage = require(path.join(ownPath, 'package.json'));
var appPackage = require(path.join(appPath, 'package.json'));

console.log(cyan('Managing dependencies'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s call this Updating dependencies...

@@ -61,8 +64,10 @@ prompt(
fs.mkdirSync(path.join(appPath, 'config', 'jest'));
fs.mkdirSync(path.join(appPath, 'scripts'));

console.log();
console.log(cyan('Copying files'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s add ... at the end of every group title


console.log('Updating scripts');
console.log();
console.log(cyan('Updating scripts'));
Copy link
Contributor

@gaearon gaearon Sep 27, 2016

Choose a reason for hiding this comment

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

Same here, let’s add ...

@@ -107,20 +112,21 @@ prompt(
true
);

console.log('Writing package.json');
console.log();
console.log(cyan('Writing ') + 'package.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

var ownPackageName = ownPackage.name;
console.log('Removing dependency: ' + ownPackageName);
console.log(red(' Removing dependency: ') + ownPackageName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s use yellow here so it doesn’t feel like an error

@gaearon gaearon added this to the 0.7.0 milestone Sep 27, 2016
@ghost ghost added the CLA Signed label Sep 27, 2016
@azakordonets
Copy link
Contributor Author

Done. Here's how it looks now :

screenshot 2016-09-27 18 00 00
screenshot 2016-09-27 18 00 09
screenshot 2016-09-27 18 00 16


console.log('Updating scripts');
console.log();
console.log(cyan('Updating scripts...'));
delete appPackage.scripts['eject'];
Object.keys(appPackage.scripts).forEach(function (key) {
appPackage.scripts[key] = appPackage.scripts[key]
Copy link
Contributor

@gaearon gaearon Sep 27, 2016

Choose a reason for hiding this comment

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

Let’s print each step here?
For example:

  Replacing "react-scripts start" with "node scripts/start.js"

files.forEach(function(file) {
console.log('Copying ' + file + ' to ' + appPath);
console.log(cyan(' Copying ') + file+ ' to ' + appPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try making file and appPath cyan instead of Copying?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the output, it prints the same giant appPath on every single line.

Suggestion: could we put this path in the header instead?

Copying files to /Users/.../tempApp ...
  Copying .babelrc
  Copying .eslintrc

delete appPackage.devDependencies[ownPackageName];

Object.keys(ownPackage.dependencies).forEach(function (key) {
// For some reason optionalDependencies end up in dependencies after install
if (ownPackage.optionalDependencies[key]) {
return;
}
console.log('Adding dependency: ' + key);
console.log(cyan(' Adding dependency: ') + key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar, let’s make dependency name cyan (or yellow) instead of the label.

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2016

Sorry for the back-and-forth. Visual work takes some iterations. Can you try my suggestions?

@azakordonets
Copy link
Contributor Author

No problem at all ! I'm very happy that i can contribute to the open source project that I'm using myself :) I've made adjustments you've mentioned and he's how it looks now :

screenshot 2016-09-28 11 47 34
screenshot 2016-09-28 11 47 43
screenshot 2016-09-28 11 47 50

@montogeek
Copy link
Contributor

Can we show Jest, Babel and ESLint output too?
Example:
Updating Jest config...

@azakordonets
Copy link
Contributor Author

Done. Added.

@montogeek
Copy link
Contributor

Thank you @azakordonets :)

delete appPackage.scripts['eject'];
Object.keys(appPackage.scripts).forEach(function (key) {
appPackage.scripts[key] = appPackage.scripts[key]
.replace(
new RegExp(ownPackageName + ' (\\w+)', 'g'),
'node scripts/$1.js'
);
console.log(' Replacing react-scripts ' + cyan(key) + ' with ' + cyan(appPackage.scripts[key]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make it so react-scripts is also cyan here. We're replacing a complete command.
Also let's put both "react-scripts start/test/build" and "node scripts/whatever.js" in quotes.

  Replacing "react-scripts start" with "node scripts/start.js"

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

This looks good to me. I left a comment about the last change I want to make.
Please also resolve the merge conflicts.

Thank you for working on this!

@azakordonets
Copy link
Contributor Author

Ok, i pushed this changed and it looks like this :

screen shot 2016-09-30 at 15 11 38

@azakordonets
Copy link
Contributor Author

As for the merge conflicts, i don't have enough permissions to resolve conflicts.
screen shot 2016-09-30 at 15 12 30

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

To resolve conflicts:

git remote add facebookincubator https://github.com/facebookincubator/create-react-app.git
git fetch facebookincubator
git merge facebookincubator/master

You'll see the conflicts. After you resolve them, push the branch again.

@azakordonets
Copy link
Contributor Author

Done. You can check now. Since updating of .babelrc and .eslint files was taken out into separate commands, i also added console.log for them. Output looks like this now :
screen shot 2016-09-30 at 16 05 40

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

I like everything except the very last section.
Can we change it to be

Adding configuration to package.json...
  Adding Babel preset
  Adding ESLint configuration
  Adding Jest configuration

I also think we can completely omit Writing package.json since it's not very descriptive.

@azakordonets
Copy link
Contributor Author

Would you like to have it all in cyan color, or should i only use cyan for "Adding" word ?

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

All cyan is fine.

@azakordonets
Copy link
Contributor Author

What do you think about such formatting :
screen shot 2016-09-30 at 16 36 16
Or you would preffer to put it all Cyan ?

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

This looks good to me!

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

The only nit would be that ... after package.json shouldn't be white since it's not part of the filename

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

Also quotes in "node scripts/..." should be cyan, just like quotes in "react-scripts ...".

@azakordonets
Copy link
Contributor Author

Done. Looks like this :
screen shot 2016-09-30 at 18 54 55

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

This looks awesome. Thanks for sticking with it.
I’ll wait for Travis and merge!

@azakordonets
Copy link
Contributor Author

Sweeet :) My pleasure . I'm very new to Javascript and React, so it's really great that i managed to contribute to this cool project. I checked other issues with "Help wanted" label, but they don't seem easy for me to pick up for now, but i'll continue tracking this project. Thanks for oportunity

@gaearon gaearon merged commit 27e76be into facebook:master Sep 30, 2016
@gaearon gaearon mentioned this pull request Sep 30, 2016
kitze added a commit to kitze/custom-react-scripts that referenced this pull request Oct 3, 2016
…react-app

# By Dan Abramov (5) and others
# Via Dan Abramov
* 'master' of https://github.com/facebookincubator/create-react-app:
  docs(readme): peer dependencies applied (facebook#818)
  Fix typos on ISSUE_TEMPLATE.md (facebook#817)
  Add explicit linebreaks (facebook#813)
  Fix typo (facebook#810)
  Fix some typos (facebook#809)
  Beaufity output of eject.js script (facebook#769)
  Define process.env as object (facebook#807)
  Typo fix in webpack.config.dev.js comments (facebook#777)
  Add Netlify to deploy instructions
  Fix usage example to match react-dev-utils@0.2.x API
  Relaxed eslint rule no-unused-expressions (facebook#724)
  Fix the doc
  Publish
  Add 0.6.1 changelog
  Moved Babel and ESLint config to package.json after ejecting (facebook#773)

Conflicts:
	packages/react-scripts/package.json
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Beaufity output of eject.js script

* change formatting of the eject.js output and move colors to cyan

* change message about file copy

* add missing three dots to some statements in eject.js script

* change color of "copying files" line and do not repeat copy path anymore in log

* fix merge conflict

* Remove yellow color from "Removing dependency" line

* changing color to "Adding dependency" line

* Add line that outputs which react script is getting replaced by similar node script

* remove not used anymore colors

* add console line about updating Jest configs

* fix typo

* change formatting of replacing script output in eject.js

* remove "Writing package.json" file console output

* make quotes cyan in "Replacing script" console output

* update console log output for Jest, Babel, ESLint update and group them under one statement

* Style nits
maltestenzel pushed a commit to maltestenzel/custom-react-scripts that referenced this pull request Mar 7, 2018
…react-app

# By Dan Abramov (5) and others
# Via Dan Abramov
* 'master' of https://github.com/facebookincubator/create-react-app:
  docs(readme): peer dependencies applied (facebook#818)
  Fix typos on ISSUE_TEMPLATE.md (facebook#817)
  Add explicit linebreaks (facebook#813)
  Fix typo (facebook#810)
  Fix some typos (facebook#809)
  Beaufity output of eject.js script (facebook#769)
  Define process.env as object (facebook#807)
  Typo fix in webpack.config.dev.js comments (facebook#777)
  Add Netlify to deploy instructions
  Fix usage example to match react-dev-utils@0.2.x API
  Relaxed eslint rule no-unused-expressions (facebook#724)
  Fix the doc
  Publish
  Add 0.6.1 changelog
  Moved Babel and ESLint config to package.json after ejecting (facebook#773)

Conflicts:
	packages/react-scripts/package.json
@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.

5 participants