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

Open local address in default browser #198

Merged
merged 10 commits into from
May 23, 2017
Merged

Open local address in default browser #198

merged 10 commits into from
May 23, 2017

Conversation

webtaculars
Copy link
Contributor

@webtaculars webtaculars commented May 18, 2017

Open local address in default browser by using option --local.
serve --local

EDIT
Command is now serve --open

@leo
Copy link
Contributor

leo commented May 18, 2017

👍 I'd rather call it --open

@webtaculars
Copy link
Contributor Author

webtaculars commented May 18, 2017

Yeah 😀. It should be --open. I will update it.

@webtaculars
Copy link
Contributor Author

@leo I have changed --local to --open. Need to update anything else?

lib/listening.js Outdated
@@ -67,6 +68,13 @@ module.exports = coroutine(function*(server, current, inUse, clipboard) {
} catch (err) {}
}

if(open) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a space before the (

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please test for isTTY

lib/listening.js Outdated
if(open) {
try {
opn(localURL)
message += `\n\n${chalk.grey('Local address has opened in browser!')}`
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 not show any additional messages in the terminal!

bin/serve.js Outdated
@@ -108,7 +109,7 @@ detect(port).then(open => {
server.listen(
port,
coroutine(function*() {
yield listening(server, current, inUse, flags.noClipboard !== true)
yield listening(server, current, inUse, flags.noClipboard !== true, flags.open === true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the comparison? flags.open should be enough

@webtaculars
Copy link
Contributor Author

@leo Updated all the changes you asked.

bin/serve.js Outdated
@@ -50,7 +50,7 @@ args
)
.option('silent', `Don't log anything to the console`)
.option('no-clipboard', `Don't copy address to clipboard`, false)

.option('open', `Open local address in browser`, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ' instead of ` here, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I change it for silent and no-clipboard also?

Copy link
Contributor

Choose a reason for hiding this comment

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

@webtaculars No, because they have a ' in the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can escape it using backslash

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but prettier might revert that. Try it!

@leo leo merged commit e7a3c36 into vercel:master May 23, 2017
@leo
Copy link
Contributor

leo commented May 23, 2017

Thanks! ✌️

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.

None yet

2 participants