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

Refactor/upgrade create react app #21

Merged
merged 16 commits into from
Jul 4, 2018

Conversation

asbjornh
Copy link
Contributor

@asbjornh asbjornh commented Jun 26, 2018

Upgrades to @creuna/create-react-app@2.0.0

  • Adds prompt for running create-react-app
  • Adds prompt for and file writing of .vscode/tasks.json
  • Upgrades other dependencies

Fixes #20

Copy link

@Reasonable-Solutions Reasonable-Solutions left a comment

Choose a reason for hiding this comment

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

I am wondering about the tasks bit, according to the vscode docs they autodetect npm tasks? Could we have them live there instead of shipping some pretty editor specific stuff?

source/emoji.js Outdated
@@ -2,6 +2,8 @@

module.exports = function(emoji, fallback = '') {
return process.stdout.isTTY && process.platform === 'darwin'
? `${emoji} `
? emoji

Choose a reason for hiding this comment

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

nested ternary?
not fan, do we have a policy on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, but I removed it for you. Better?

@asbjornh
Copy link
Contributor Author

As far as I know, there’s no way of passing input when you use npm scripts within code. The vs code tasks file lets you run a script on the file that is currently open in the editor via some environment variables.

If you can find a way to do the same without tasks.json we should switch to that. Until then I want it to stay

@pellebjerkestrand
Copy link
Contributor

@creuna/create-react-app@2.0.1 is approved. Should we wait a bit and upgrade to that release in this PR?

@asbjornh
Copy link
Contributor Author

Sure. We could also fait for Creuna-Oslo/react-scripts#13

@asbjornh
Copy link
Contributor Author

@pellebjerkestrand Now updated to v2 of react-scripts as well.

I also moved the logging of help when command is missing to the top of index.js. It now prints the help around 1 second faster

@asbjornh
Copy link
Contributor Author

Tested on windows in Powershell. Everything but lib seems to be working. lib is also broken on master. Seems to be an issue with select-shell. I'll create an issue

@asbjornh
Copy link
Contributor Author

asbjornh commented Jul 4, 2018

Both I and @serling have tested lib on windows and now it seems to be working. I think it's time to merge this, so that's what I'm going to do :)

@asbjornh asbjornh merged commit ca8c656 into master Jul 4, 2018
@asbjornh asbjornh deleted the refactor/upgrade-create-react-app branch July 4, 2018 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants