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

automatically open webdash url on serve #66

Merged
merged 2 commits into from
Apr 1, 2018
Merged

automatically open webdash url on serve #66

merged 2 commits into from
Apr 1, 2018

Conversation

bardiarastin
Copy link
Contributor

this is for #64

@bardiarastin bardiarastin changed the title automatically open webdash url on sever automatically open webdash url on seve Mar 30, 2018
@bardiarastin bardiarastin changed the title automatically open webdash url on seve automatically open webdash url on serve Mar 30, 2018
@jadjoubran jadjoubran self-assigned this Mar 30, 2018
@jadjoubran jadjoubran added the enhancement New feature or request label Mar 30, 2018
@jadjoubran jadjoubran added this to the 1.3 milestone Mar 30, 2018
@jadjoubran
Copy link
Owner

it doesn't seem to be working for me
Here's what I get:

screen shot 2018-03-31 at 12 41 29 am

I'll debug it tomorrow, it may be related to this opn#86

Note: when developing webdash locally, I use node ./scripts/index.js --local (kinda similar to npx webdash serve)

jadjoubran
jadjoubran reviewed Mar 31, 2018
View reviewed changes
scripts/index.js Outdated
@@ -23,6 +23,7 @@ const serveCommand = argv => {
const url = chalk.underline(`http://${host}:${port}`);
const message = chalk.keyword('green').bold(`Webdash running on ${url}!`);
console.log(message);
opn(url)
Copy link
Owner

Choose a reason for hiding this comment

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

okay here's why it's not working:
url is formatted text from chalk.underline. So you want to save the url before formatting it, and then after formatting it
and opn should only take url (unformatted)

also please wrap opn(url) with a try catch:

try {
opn(url);
} catch(error){}

because if it didn't work, it's not a big deal, and users might think that webdash is broken if opn broke (whereas webdash is still working)

@jadjoubran
Copy link
Owner

okay I found the issue 😄
Thanks for your PR!

@jadjoubran jadjoubran added the cli label Mar 31, 2018
@bardiarastin
Copy link
Contributor Author

i’ll fix this today

@bardiarastin
Copy link
Contributor Author

fix is here 37be3ac

@jadjoubran jadjoubran merged commit 08340b9 into jadjoubran:master Apr 1, 2018
@jadjoubran
Copy link
Owner

Thanks! 😄
I will do some minor variable names renaming and then publish v1.3 👉 #69

@bardiarastin
Copy link
Contributor Author

Pretty fast 😄

@jadjoubran
Copy link
Owner

😄

@bardiarastin
Copy link
Contributor Author

can you please wait for jadjoubran/webdash-npm-scripts#6 and then we release ? pull request in a few hours

@jadjoubran
Copy link
Owner

Sure no rush btw! I can release webdash without having to publish all the other plugins
or I can even publish 1.3 of webdash-npm-scripts and then release 1.3.1 (patch) for you PR
I don't think the plugins will remain aligned for a long time (it's not a requirement)
Thanks again for your time!

@bardiarastin
Copy link
Contributor Author

would you please create a community in https://gitter.im for webdash so anyone contributing can talk easily ?

@jadjoubran jadjoubran mentioned this pull request Apr 1, 2018
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jadjoubran jadjoubran jadjoubran left review comments

Assignees

@jadjoubran jadjoubran

Labels
cli enhancement New feature or request
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.
2 participants
@bardiarastin @jadjoubran