Issue 6182 - Add build step for fonts and images in help.eyeo.com

Issue 6182 - Add build step for fonts and images in help.eyeo.com

[ire, 2017-12-13 12:15:44]

Ready for review

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode83
gulpfile.js:83: return gulp.src(["./static/src/fonts/**"])
I couldn't find any information on build/optimisation steps for fonts. I added
this to optimise the svg font files

[juliandoucette, 2017-12-13 16:22:48]

Relevant: https://gitlab.com/eyeo/websites-docs/issues/1

I'm hesitant to add an image optimization step to our build process because I
think it implies that we should push unoptimized images to our source code
repositories (which will make them heavier). I think the solution is to separate
our image repositories from our code repositories. But I may be over-thinking
this. What do you think?

[ire, 2017-12-14 09:07:33]

On 2017/12/13 16:22:48, juliandoucette wrote:
> Relevant: https://gitlab.com/eyeo/websites-docs/issues/1
> 
> I'm hesitant to add an image optimization step to our build process because I
> think it implies that we should push unoptimized images to our source code
> repositories (which will make them heavier). I think the solution is to
separate
> our image repositories from our code repositories. But I may be over-thinking
> this. What do you think?

I think the ideal development workflow would be what I have suggested here,
which is where we drop any/unoptimized images and the build process optimises
them. But you bring up a really good point. It seems a bit wasteful/unnecessary
to push the unoptimized versions. But I don't think that means we can't achieve
this as a step in the build process. The gulp task could also replace the src
versions of the images. (Although that may be a bit weird too, to alter contents
of the src directory). I just think it should be something that happens
(essentially) automatically, and without adding an extra step.

[juliandoucette, 2017-12-14 13:01:45]

> I just think it should be something that happens
> (essentially) automatically, and without adding an extra step.

I agree.

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode66
gulpfile.js:66: .pipe(imagemin([
This could be an expensive process. I wonder if we can/should make it optional
and/or can use cache somehow?

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode83
gulpfile.js:83: return gulp.src(["./static/src/fonts/**"])
On 2017/12/13 12:15:43, ire wrote:
> I couldn't find any information on build/optimisation steps for fonts. I added
> this to optimise the svg font files

I wonder if this is necessary / how much overhead it adds :/

[ire, 2017-12-16 09:59:25]

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode66
gulpfile.js:66: .pipe(imagemin([
On 2017/12/14 13:01:44, juliandoucette wrote:
> This could be an expensive process. I wonder if we can/should make it optional
> and/or can use cache somehow?

If I go with my suggestion for this task to replace the contents of the `src`
directory as well, then I don't think this would be expensive (since the images
will already be optimised?). What do you think?

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode83
gulpfile.js:83: return gulp.src(["./static/src/fonts/**"])
On 2017/12/14 13:01:44, juliandoucette wrote:
> On 2017/12/13 12:15:43, ire wrote:
> > I couldn't find any information on build/optimisation steps for fonts. I
added
> > this to optimise the svg font files
> 
> I wonder if this is necessary / how much overhead it adds :/

Probably not necessary, just thought to mention it in case you had any ideas.

[juliandoucette, 2017-12-18 17:11:17]

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode66
gulpfile.js:66: .pipe(imagemin([
On 2017/12/16 09:59:24, ire wrote:
> If I go with my suggestion for this task to replace the contents of the `src`
> directory as well, then I don't think this would be expensive (since the
images
> will already be optimised?). What do you think?

Are you suggesting that we run this task to automatically optimize images before
we commit them?

If so, that seems like a case against "src" and "dist" directories (because we
can add "min" to js/css and src images will no longer be *just* src images).

(I don't think it really matters whether or not we have "src"/"dist" as long as
src files and source maps are accessible.)

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode83
gulpfile.js:83: return gulp.src(["./static/src/fonts/**"])
I think we can exclude this then (and add it later if necessary).

[ire, 2017-12-20 08:44:21]

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode66
gulpfile.js:66: .pipe(imagemin([
On 2017/12/18 17:11:16, juliandoucette wrote:
> On 2017/12/16 09:59:24, ire wrote:
> > If I go with my suggestion for this task to replace the contents of the
`src`
> > directory as well, then I don't think this would be expensive (since the
> images
> > will already be optimised?). What do you think?
> 
> Are you suggesting that we run this task to automatically optimize images
before
> we commit them?

Yes. Since the gulp task is watching the src/img directory, any images put there
would be automatically optimised (in the src folder), plus moved to the dist
folder. 

> If so, that seems like a case against "src" and "dist" directories (because we
> can add "min" to js/css and src images will no longer be *just* src images).
> (I don't think it really matters whether or not we have "src"/"dist" as long
as
> src files and source maps are accessible.)

Yes it seems so. I think the separation of the "src" and "dist" is useful for
the css and js files, because otherwise things can get a bit messy having all
the files together (particularly for the js files since css is already separated
by scss). 

I see that the (proposed) img gulp task would be a bit of a weird application of
the src/dist folders. My inclination is to go ahead anyway, because I think it's
worth having the separation.

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode83
gulpfile.js:83: return gulp.src(["./static/src/fonts/**"])
On 2017/12/18 17:11:16, juliandoucette wrote:
> I think we can exclude this then (and add it later if necessary).

Do you mean to exclude this whole task? Or to exclude the imagemin?

[juliandoucette, 2018-01-04 15:45:36]

LGTM (Use your best judgement about the issue below).

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode83
gulpfile.js:83: return gulp.src(["./static/src/fonts/**"])
On 2017/12/20 08:44:20, ire wrote:
> On 2017/12/18 17:11:16, juliandoucette wrote:
> > I think we can exclude this then (and add it later if necessary).
> 
> Do you mean to exclude this whole task? Or to exclude the imagemin?

Exclude imagemin. Excluding the whole task would mean moving fonts from src to
dist and not ignoring dist (which I may not have a problem with, but it's
unnecessary).

[ire, 2018-01-05 07:56:10]

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js
File gulpfile.js (right):

https://codereview.adblockplus.org/29637578/diff/29637580/gulpfile.js#newcode83
gulpfile.js:83: return gulp.src(["./static/src/fonts/**"])
On 2018/01/04 15:45:35, juliandoucette wrote:
> On 2017/12/20 08:44:20, ire wrote:
> > On 2017/12/18 17:11:16, juliandoucette wrote:
> > > I think we can exclude this then (and add it later if necessary).
> > 
> > Do you mean to exclude this whole task? Or to exclude the imagemin?
> 
> Exclude imagemin. Excluding the whole task would mean moving fonts from src to
> dist and not ignoring dist (which I may not have a problem with, but it's
> unnecessary).

Ack. That's what I thought, just confirming. Done.