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

Offer to set default browsers #3792

Merged
merged 7 commits into from
Jan 14, 2018
Merged

Offer to set default browsers #3792

merged 7 commits into from
Jan 14, 2018

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Jan 14, 2018 •
edited
Loading

@Timer Timer added this to the 2.0.0 milestone Jan 14, 2018
@Timer Timer requested a review from gaearon January 14, 2018 17:35
@Timer Timer changed the base branch from master to next January 14, 2018 17:35
@Timer Timer changed the title Set default browsers Offer to set default browsers Jan 14, 2018
@gaearon
Copy link
Contributor

gaearon commented Jan 14, 2018

Nice, but we should output the defaults we've written.

gaearon
gaearon reviewed Jan 14, 2018
View reviewed changes
packages/react-scripts/scripts/start.js Outdated
.then(() =>
// We attempt to use the default port but if it is busy, we offer the user to
// run on a different port. `choosePort()` Promise resolves to the next free port.
choosePort(HOST, DEFAULT_PORT).then(port => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Return this to avoid the extra nesting level?

gaearon
gaearon reviewed Jan 14, 2018
View reviewed changes
packages/react-scripts/scripts/build.js Outdated
.then(() =>
// First, read the current file sizes in build directory.
// This lets us display how much they changed later.
measureFileSizesBeforeBuild(paths.appBuild)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as below

@Timer
Copy link
Contributor Author

Timer commented Jan 14, 2018

gaearon
gaearon approved these changes Jan 14, 2018
View reviewed changes
Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Might be a bit clearer with blocks and explicit return statements

@Timer
Copy link
Contributor Author

Timer commented Jan 14, 2018

More explicit it is: 2dc7f9f

@Timer Timer merged commit 727f487 into facebook:next Jan 14, 2018
@Timer Timer deleted the set-default-browsers branch January 14, 2018 18:54
gaearon pushed a commit that referenced this pull request Jan 14, 2018
@Timer @gaearon
Offer to set default browsers (#3792)
43bbaf0 
* Offer to set browser defaults

* Catch error on no

* Add ending newlines

* Ensure we re-check to prevent defaults from leaking

* Reduce nesting

* Add defaults message

* More explicit
Timer added a commit to Timer/create-react-app that referenced this pull request Jan 15, 2018
@Timer
Offer to set default browsers (facebook#3792)
2e59c54 
* Offer to set browser defaults

* Catch error on no

* Add ending newlines

* Ensure we re-check to prevent defaults from leaking

* Reduce nesting

* Add defaults message

* More explicit
akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
@Timer @akstuhl
Offer to set default browsers (facebook#3792)
83f2db6 
* Offer to set browser defaults

* Catch error on no

* Add ending newlines

* Ensure we re-check to prevent defaults from leaking

* Reduce nesting

* Add defaults message

* More explicit
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
@Timer
Offer to set default browsers (facebook#3792)
6a92156 
* Offer to set browser defaults

* Catch error on no

* Add ending newlines

* Ensure we re-check to prevent defaults from leaking

* Reduce nesting

* Add defaults message

* More explicit
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gaearon gaearon gaearon approved these changes

Assignees
No one assigned
Labels
CLA Signed tag: enhancement
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Timer @gaearon @facebook-github-bot