Skip to content
This repository has been archived by the owner on May 2, 2022. It is now read-only.

Replace LiveReload with Browsersync #66

Merged
merged 24 commits into from
Oct 10, 2018
Merged

Replace LiveReload with Browsersync #66

merged 24 commits into from
Oct 10, 2018

Conversation

colorful-tones
Copy link
Contributor

I've just taken what @JodiWarren has done in #65 and updated README.md and used the URL in package.json

Closes #60

JodiWarren
JodiWarren previously approved these changes Sep 11, 2018
Copy link
Contributor

@JodiWarren JodiWarren left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for jumping on those changes!

gulpfile.babel.js Outdated Show resolved Hide resolved
@timwright12 timwright12 removed the request for review from garand September 11, 2018 18:33
@timwright12
Copy link
Contributor

@colorful-tones does this PR replace #65 ?

@timwright12
Copy link
Contributor

@colorful-tones once this PR is worked out, we need to get the changes into the plugin repo before merging as well

README.md Outdated Show resolved Hide resolved
gulpfile.babel.js Show resolved Hide resolved
gulpfile.babel.js Outdated Show resolved Hide resolved
@garand
Copy link

garand commented Sep 12, 2018

@JodiWarren For the local URL issue... would using 0.0.0.0 solve that problem?

https://superuser.com/questions/949428/whats-the-difference-between-127-0-0-1-and-0-0-0-0

@JodiWarren
Copy link
Contributor

JodiWarren commented Sep 12, 2018

@garand Normally 0.0.0.0 won't resolve to a desired site. If that's how you have your local environment setup then it will absolutely work, but afaik it's not a normal setup.

If you want to quickly test this, try on the command line:
browser-sync start --proxy '0.0.0.0'
vs
browser-sync start --proxy 'garand.test'

In the (nearish) future we're going to have a wp-local-docker setup which easily enables multiple live sites at once, so we'll definitely want to be reasonably specific here.

FWIW this is just being provided to the proxy option, so for further reading: https://browsersync.io/docs/options#option-proxy

@colorful-tones
Copy link
Contributor Author

@colorful-tones does this PR replace #65 ?

Yes, this replaces #65

@colorful-tones once this PR is worked out, we need to get the changes into the plugin repo before merging as well

👍 that makes sense and will make sure to push it through to that repo once we're happy with final implementation in theme-scaffold

I'm still trying to get out best way to wrap a check for localUrl, but will push as soon as I have something. Thanks!

gulpfile.babel.js Outdated Show resolved Hide resolved
gulpfile.babel.js Outdated Show resolved Hide resolved
gulpfile.babel.js Outdated Show resolved Hide resolved
gulpfile.babel.js Outdated Show resolved Hide resolved
Copy link
Contributor

@JodiWarren JodiWarren left a comment

Choose a reason for hiding this comment

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

Looks good to me apart that one typo. I don't need to review it again after you fix that!

if ( proxyUrl ) {
// https://browsersync.io/docs/options
bs.init({
proxy: packproxyUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, I think you've got a wee typo on packproxyUrl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omg

Copy link
Contributor

Choose a reason for hiding this comment

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

Mate if that's the worst bug you make in a PR you're doing well.

and get some sleep!
@colorful-tones
Copy link
Contributor Author

I opened an Issue on the Plugin Scaffold so we can get this in there once we're satisfied with results here.

gulpfile.babel.js Outdated Show resolved Hide resolved
@timwright12
Copy link
Contributor

@colorful-tones This is good to go, let's get it into the plugin scaffold and I'll merge at the same time.

@colorful-tones
Copy link
Contributor Author

@timwright12 PR is in on Plugin Scaffold now: 10up/plugin-scaffold#58

Thanks.

@timwright12 timwright12 merged commit b4570f4 into 10up:master Oct 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants