-
-
Notifications
You must be signed in to change notification settings - Fork 302
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
NEW: ☕️ Separate Config File #66
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a buggy PR. Take a look at my comments. Haven't had a chance to try it out, traveling, low internet access. Looking forward!
@@ -139,7 +77,7 @@ function browsersync() { | |||
// @link http://www.browsersync.io/docs/options/ | |||
|
|||
// Project URL. | |||
proxy: projectURL, | |||
proxy: config.projectURL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You defined it as CONFIG
but are using it as config
that won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. My fault. I got my branches crossed up - literally.
|
||
// START Editing Project Variables. | ||
// Project related. | ||
project : 'WPGulpTheme', // Project Name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this? This doesn't look like an JS object
and neither is it a var
, let
, or const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I was not 100% familiar with this way of working either - but it seems to be utilized in large projects that use Gulp.
These articles do a good job of explaining it:
https://www.sitepoint.com/understanding-module-exports-exports-node-js/
http://stackabuse.com/how-to-use-module-exports-in-node-js/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is done wrong!
E.g. project
is nothing. It should be a const
and so does all variables that don't change the values. You seem to have removed the word var
but haven't replaced it with either let
or const
. Go with const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not wrong. It's an export module. Check out the article above.
It's similar to how Foundation sets up their config file: https://github.com/zurb/foundation-sites/blob/develop/gulp/config.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good for now to be merged. Just the spacing and indentation issues for which I think we should add eslint
and prettier
configs.
Woah. My fault. Looks like I crossed up a couple of branches I had. I updated the Gulpfile.js to use the lowercase version of |
|
||
// START Editing Project Variables. | ||
// Project related. | ||
project : 'WPGulpTheme', // Project Name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is done wrong!
E.g. project
is nothing. It should be a const
and so does all variables that don't change the values. You seem to have removed the word var
but haven't replaced it with either let
or const
. Go with const
.
If there's a better way of handling this, I'm all for it! |
Thanks for the PR @JeremyEnglert — I just tested it and merged it with the new branch. Way to go! |
No other changes made beyond moving the current variables/configuration into a separate config.js file
FIX #65