Skip to content

Conversation

@allanjamesvestal
Copy link
Contributor

@allanjamesvestal allanjamesvestal commented Mar 17, 2018

This PR is meant to demonstrate one way we can simplify the image resizing, minifying and copying (to ./dist/) flow that currently takes us three separate steps, each with its own Gulp task.

It's not necessarily the perfect way forward, but it's at least a start, and my hope is it sparks a discussion of how we ought to approach replacing what Andrew and I have found to be an overwrought, unapproachable tangle of logic.

In this branch, there's now just one image related task on the page generator: img.js. It defines individual configuration lines for dealing with files in ./src/images/, where each configuration line represents one specific desired output size (or lack thereof, in which case files are transferred over at their original resolution) per each file format (JPG, PNG or other) and matching pattern (with underscores or without).

This lets us have configuration lines that say the following:

  • For .png files that begin with an underscore, minify them and move them over to ./dist/ at their current size.
  • For .jpg files that don't begin with an underscore, create a minified, resized copy at 600px wide and place that file in ./dist/, with a filename suffixed by its new width.
  • As the next configuration line, find all the same .jpg files (the ones without an underscore at their beginning) and create a minified, resized copy at 1200px wide. Give those files a suffix of filename-1200 and place them next to the 600px wide copies in ./dist/.
  • For all non-JPGs or PNGs in ./src/images/, move them into ./dist/ without any minification or resizing.

Current issues

The only known issues at present concern the order in which this operates. The old flow used to "optimize" (or minify, using the same gulp-imagemin/imagemin-jpeg-recompress library we've retained in this branch) once, and then resize to each needed width based on the optimized version, which was saved in ./src/images/opt/.

This was kind of an anti-pattern, as it meant you had to monitor both the files at the top level of ./src/images/ and those one folder deeper in ./src/images/opt/, potentially deleting both whenever you wanted to update an image.

The new way resizes and then minifies, which is easier to do programmatically in one step. But it may present a couple challenges:

  1. It takes (slightly) longer to generate each thumbnail size of an image, since it's running two steps on each one. As discussed with Andrew earlier, I think that trade-off is worth it for the increased simplicity. But, I want to make sure the image quality doesn't suffer by doing it this way.
  2. It prints more output in the console. That's maybe not ideal, but I'm not entirely concerned about that either.
  3. It re-renders all the images every time Gulp is run. This is probably the biggest deal-breaker on large projects, but my hope is we can use some shortcuts (like gulp-changed, which the older image tasks used heavily) to avoid un-necessary regeneration of images.

That all said, I'd definitely be curious what y'all think about this change going forward.

Prior art

Much of this branch owes to the work of this Gist's author, who did a very similar thing to this logic several years ago — using the same image-minifying and resizing libraries we had before, so the transition was extremely easy.

I mainly include it here so you can see how he did things, and what my thought process was coming into this myself.

https://gist.github.com/ryantbrown/239dfdad465ce4932c81

@allanjamesvestal
Copy link
Contributor Author

Also, I forgot to switch back into master before starting this, so let's definitely merge the first two commits (which Andrew has on another branch) first. That way, this should only show up as my three commits, plus whatever else we add on here before we actually incorporate this code.

@johndhancock
Copy link
Contributor

Regarding your concerns, the re-rendering of each image each time was the main reason Jon set it up this way after discussing with me. I'd prefer that not happen.

I don't think the resize then optimize pattern should reduce file quality. Typically, that's what I would do in the old days (resize to the size I wanted in photoshop, then save for web), but yes, we should test that to make sure.

@johndhancock
Copy link
Contributor

johndhancock commented Aug 1, 2018

Circling back around on this. Did some testing this morning. There's no discernible difference in image quality that I can tell with the naked eye, though it does look like the images outputted by the unified branch are about 50% larger in file size. In my opinion, concerns #1 and #2 listed above aren't enough to not do this. The bigger question for me is concern #3, and do the tradeoffs of having to wait for that process to run on every gulp startup outweigh the issues this PR is trying to solve, along with can we get the file sizes in line between the two versions.

In most instances, I think it'll be a minor annoyance at most, but in cases of projects like Rural Royalty, which is an outlier, but has 100+ images in it, this could be a significant burden to startup.

In any case, let's make a call on what we think is best and either close the PR or merge it in.

@achavez
Copy link
Contributor

achavez commented Aug 3, 2018

Would something like this help with the startup slowness? https://github.com/segmentio/concurrent-transform

@johndhancock
Copy link
Contributor

@allanjamesvestal I'm fine with merging this in and seeing how things go afterward. I'd very much like for you to take a look at what Andrew suggested and give your thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants