Added imagemin to gulp #1104
Added imagemin to gulp #1104
Conversation
|
LGTM but needs a review from gulp users |
|
👍 |
| @@ -92,6 +92,7 @@ | |||
| "gulp-csslint": "~0.1.5", | |||
| "gulp-cssmin": "~0.1.7", | |||
| "gulp-eslint": "~1.0.0", | |||
| "gulp-imagemin": "2.4.0", | |||
|
Which task runs this? By default, I think it makes sense to have a build/dist task include this, no? |
| @@ -35,6 +35,9 @@ module.exports = { | |||
| 'modules/*/client/*.js', | |||
| 'modules/*/client/**/*.js' | |||
| ], | |||
| img: [ | |||
| 'modules/users/client/img/**/*.png' | |||
There was a problem hiding this comment.
We only want to support minifying .png?
|
+1 on adding this to the |
|
thanks for the feedback guys! I added |
| @@ -35,6 +35,9 @@ module.exports = { | |||
| 'modules/*/client/*.js', | |||
| 'modules/*/client/**/*.js' | |||
| ], | |||
| img: [ | |||
| 'modules/**/**/img/**/*' | |||
There was a problem hiding this comment.
You can still specify specific images, I'd rather not just let this bluntly run on any file in there because there may be user error.
There was a problem hiding this comment.
ok... should we just only handle .png, .jpg, .gif and .ico file types?
There was a problem hiding this comment.
Handle whichever ones the png library supports? Anything else and we may have an error, so we can just avoid that by using the multiple term matching syntax, which I think is (abc|xyz|def), but verify that.
|
Ok, so we compress images as part of the build process - wasted disk space and build time. nothing is even referencing these files right now. Why does this need to be a part of the build process? Seems a bit opinionated? Configuration is way too broad. What if I don't care to compress my images? I really feel like this would be better served as a utility, not part of the build process - one offs. |
| @@ -11,9 +11,11 @@ var _ = require('lodash'), | |||
| runSequence = require('run-sequence'), | |||
| plugins = gulpLoadPlugins({ | |||
| rename: { | |||
| 'gulp-angular-templatecache': 'templateCache' | |||
| 'gulp-angular-templatecache': 'templateCache', | |||
| 'gulp-imagemin': 'imagemin' | |||
|
@accraze any progress on this? |
|
@accraze can you finalize this PR? |
|
sure no prob... did we ever reach a consensus on whether or not to include Aside from the coveralls issues, imagemin is ready to go and supports jpg, png, gif and svg. Will send a fix later today |
|
Let's not add it to the build task. Someone could easily add it to the build sequence if they wish to. |
accraze
force-pushed
the
gulp-imagemin
branch
from
11787ca to
d50f6b7
Compare
|
cool... removed imagemin from the build task, fixed the coveralls issue and squashed all the commits... lemme know if you guys need anything else |
|
Looks like there's quite a bit of diffs that have already been merged into master. You'll need to rebase and then squash. |
|
Are all the new changes because you merged to resolve conflicts? |
|
yeah sorry I should have mentioned.... I had to merge master in order to get the coveralls tests to pass... it was failing due to missing test coverage. |
|
yeah no prob... will fix later this evening |
|
@accraze I'll be on Gitter meanjs most of the day & evening, if you need tips on doing the rebase. |
accraze
force-pushed
the
gulp-imagemin
branch
from
d50f6b7 to
b44041c
Compare
|
took the merge commits out with a rebase last night... should be good to go |
|
LGTM. @ilanbiala @rhutchison |
| @@ -35,6 +35,12 @@ module.exports = { | |||
| 'modules/*/client/*.js', | |||
| 'modules/*/client/**/*.js' | |||
| ], | |||
| img: [ | |||
| 'modules/**/**/img/**/*.jpg', | |||
There was a problem hiding this comment.
2nd double asterisk is unnecessary, just single asterisk.
|
Nice, just 2 fixes then I'll merge. |
in order to minify images, gulp is now given an imagemin task
accraze
force-pushed
the
gulp-imagemin
branch
from
b44041c to
1a3f6f8
Compare
|
@ilanbiala... fixed! |
This PR is in reference to #1066 (Grunt/Gulp tasks)
in order to minify images in the build process,
gulp is now given an imagemin task.