Skip to content
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

update responsive image processing #144

Merged
merged 20 commits into from
Jul 14, 2021
Merged

update responsive image processing #144

merged 20 commits into from
Jul 14, 2021

Conversation

dvdherron
Copy link
Contributor

Cute animal pic

image

Because everyone likes pictures of animals.

Link to Trello card (if applicable)

https://trello.com/c/UgC5TdR0/66-consider-removing-gulp-update-responsive-images
If this PR relates to a Trello card, don't forget to reference this PR on the card as well.

Description

This is an effort to replace using Gulp to process responsive images. Maybe we can do this instead with the 11ty images plugin?
The commit messages say what you did; this should explain why and/or how.

  • [ x] Description is in Trello card

Steps to test/reproduce

To come
Please explain how to best reproduce the issue and/or test the changes locally (including the pages/URLs/views/states to review).

Show me

To come
Provide screenshots/animated gifs/videos if necessary.

@dvdherron
Copy link
Contributor Author

@dvdherron
Copy link
Contributor Author

@jerivas Didn't see this note until today. Maybe this was part of the reason the image failed to load?
image

We can set this up with synchronous usage instead. I haven't set this up in the eleventy.js file just yet. Need to figure out how to some solve some of the Husky linting errors to push those changes here.

@jerivas
Copy link
Member

jerivas commented May 4, 2021

@dvdherron great! Let's try the sync version and see if that works 👍

jgerigmeyer and others added 5 commits May 11, 2021 14:29
* main: (30 commits)
  😞
  use netlify.toml
  new tack
  fine python 3.7 geez netlify 🙄
  python-3.8
  keep trying python versions...
  Try specifying python version for netlify and circleci
  Upgrade dependencies.
  add missing redirect
  review
  review
  lint
  add link to mention of style containment
  Remove jobs from nav, and update about/tools urls
  F-Word and FEDSA
  Use "range" for macros, not for filter
  More consistent names for custom date formats.
  draft edit to cq post
  Replace custom date formatting with date-fns
  fix oss &
  ...
@dvdherron
Copy link
Contributor Author

@jgerigmeyer @mirisuzanne

I've done the first part of editing the img macro to use the 11ty image shortcode and it partially works. I think I'll need some assistance on the last pieces to get this working all the way.

  • Images are loaded with the picture element and the correct file sources
  • It takes a while for the images to process, so they are slow to load in the browser
  • I commented out the show_attrs utility macro for now until we find a new place to call that macro (we would no longer need that . This removes some styling from some of the images.

@mirisuzanne
Copy link
Member

I updated it in a few ways (sorry if I got carried away here, I was figuring it out as I went):

  • It uses the settings we had set in taxonomy.yaml
  • I removed the old image logic, and cleaned up what had been commented in the macro
  • I added a filter for getting the url only, no picture element (this currently grabs the largest jpeg which might not be ideal?)
  • I added a way to pass in arbitrary attrs
  • I set defaults for picture & source in our css

I also moved all the blog-post images to use the embed macro, but that came with an unfortunate discovery: We're now getting a wrapper paragraph around every image — which ruins any extend-* values. I would guess that's also what happens in the figure/p issue on trello, but browsers don't allow a figure inside a paragraph, and so they generate duplicate p's on either side in the final DOM. That seems like an issue in markdown-it, and I'm not sure if we can solve it, or need a way to get around it? Maybe @jgerigmeyer has thoughts…

@mirisuzanne
Copy link
Member

(not sure how I broke the build here… 👀)

* main:
  Clarify a few places in the support-unknown article
  Update date
  Apply suggestions from code review
  adjust padding for narrow screens
  adjust font-size on narrow screens
  add basic styles for tables
  Hero image for support
  Improve support table styles, and add context
  trim trailing whitespace by default
  Compile (most of) page-specific Sass
  Side-step the comma-quotation problem
  review
  Tighten up that language, Mia
  Use ems for code font-size, to work in any context
  Remove general table styles from page style
  typo
  First draft of support unknown article
@jgerigmeyer
Copy link
Member

(not sure how I broke the build here… 👀)

Well yarn eleventy took 7m 50s for me to run locally, so I'm pretty confident the build is timing out.

.eleventy.js Outdated Show resolved Hide resolved
@jgerigmeyer
Copy link
Member

I also moved all the blog-post images to use the embed macro, but that came with an unfortunate discovery: We're now getting a wrapper paragraph around every image — which ruins any extend-* values. I would guess that's also what happens in the figure/p issue on trello, but browsers don't allow a figure inside a paragraph, and so they generate duplicate p's on either side in the final DOM. That seems like an issue in markdown-it, and I'm not sure if we can solve it, or need a way to get around it? Maybe @jgerigmeyer has thoughts…

@mirisuzanne There's not an obvious "fix" for this, but there are various workaround options. The markdown spec is that newlines are <p> tags unless otherwise specified, so it's naively doing the "right" thing here. But I think the best options for workarounds are:

  1. Wrap the macro call in <div> or <figure>
<div>{{ embed.img(...) }}</div>

Or if we change what the img macro outputs:

<figure>{{ embed.img(...) }}</figure>
  1. Try to use mdInline instead, which does not create <p> tags.

See:

@mirisuzanne
Copy link
Member

@jgerigmeyer any idea what's wrong with the yarn install, or how to fix that?

I think the extra p-tags issue is a good improvement to work on at some point - but I didn't see anything that looks actively broken. I don't think we need to hold up merge for that.

@jgerigmeyer
Copy link
Member

@jgerigmeyer any idea what's wrong with the yarn install, or how to fix that?

@mirisuzanne I didn't see any issues with the yarn install. It's the build itself (yarn build or yarn eleventy) that is now extremely slow. I didn't look into it, but we'll need to explore whether the plugin has options for better performance or caching or something.

@mirisuzanne
Copy link
Member

I didn't look into it, but we'll need to explore whether the plugin has options for better performance or caching or something.

Looks like there's an open issue requesting better caching, and some proposals for temporary solutions (including how to tie into Netlify features).

@dvdherron
Copy link
Contributor Author

@jgerigmeyer @mirisuzanne
As far as the caching issue goes . . . I tried the temporary fix mentioned here.

Even after moving the output directory for images outside of _site (so that img folder doesn't get deleted on every build) it felt like the build kept timing out. Some images were processed and some were not.

Would looking into adjusting the Sharp constructor options help?

@dvdherron
Copy link
Contributor Author

Some questions/notes from my pairing session with @oluoluoxenfree today.

I think the image processor is attempting to make four file sizes (480, 960, 1280, 2240) for every file format (jpeg, avif, webp).

This could be one of the areas where the build is getting stuck. We would need to experiment, but maybe editing the number of file types and file sizes we want could bring down the build time?

@mirisuzanne
Copy link
Member

@dvdherron @oluoluoxenfree that makes sense.

  • Maybe look into the support & performance of the formats, and propose if we should drop any of them?
  • I don't think there's any clear logic to the 4 sizes we chose. Propose a set of three(?) that would make sense?

@dvdherron
Copy link
Contributor Author

An update here @mirisuzanne (and looping you in for your input too @stacyk )

  • I think the issue that was causing the build to timeout was .avif files taking a long time to process. Without .avif files as a file-type option, the local site runs a lot smoother and more images get processed.
  • Even without .avif files, though, it seems like something is preventing every image from being processed completely. If you look into the built images in /_site/img, some of file sizes will be missing (there should be a 480, 960, 1280, 2240 size for each image)
  • There is a chance that we have too many files for this plugin at the moment?
  • One other remaining thing to try again is this cache solution. The last time I hooked this up and tried it out I still had .avif as a file format option and it didn't seem to make a difference.

With that said, most images are being processed, but with some issues:

  • In general images seem less sharp. I don't think it's an image quality setting. Maybe the source sizes in taxonamy.yaml need to be adjusted?

@dvdherron dvdherron requested a review from stacyk July 7, 2021 18:11
@stacyk
Copy link
Member

stacyk commented Jul 7, 2021

I feel like the 2240 is a really really large image size so if we are cutting down, maybe we can say we have a smaller max image size... like 1600 or something? I don't know what perf experts are suggesting these days. I think cutting it down to 3 is a good idea? At least for the build process it sounds good. I could be wrong about the size our audience loads/bandwidth our site takes up.

I haven't looked into anything with avif (thought it was for video to be honest until just now) but will read this: https://jakearchibald.com/2020/avif-has-landed/ in a few

@dvdherron You mentioned that not all image sizes are being created after it is processed, is that because the original image isn't large enough? Does this plugin try to make an image at 2240 even if the image is only 900px wide? I have to look into all of this to be able to ask the right questions, but those are a few off the top of my head. I will pull this branch and test what we have here later today. :)

@dvdherron
Copy link
Contributor Author

@dvdherron You mentioned that not all image sizes are being created after it is processed, is that because the original image isn't large enough? Does this plugin try to make an image at 2240 even if the image is only 900px wide? I have to look into all of this to be able to ask the right questions, but those are a few off the top of my head. I will pull this branch and test what we have here later today. :)

@stacyk I realized this when I woke up this morning (🙈). It will only make sizes that it can make without stretching the image. I don't know if I understand how that's working on OddSite currently. Images don't look stretched?

I feel like the 2240 is a really really large image size so if we are cutting down, maybe we can say we have a smaller max image size... like 1600 or something? I don't know what perf experts are suggesting these days. I think cutting it down to 3 is a good idea?

Yeah, I've been looking around to see what other sites do and some do even more than four sizes. I think something like 1600 or 1900 could work if we get cut it down to just three sizes.

I haven't looked into anything with avif (thought it was for video to be honest until just now) but will read this: https://jakearchibald.com/2020/avif-has-landed/ in a few

👍🏽

@stacyk
Copy link
Member

stacyk commented Jul 9, 2021

@dvdherron so I've been thinking about this since I reviewed Sondra's PR that removed the bg from images a week or two ago... I thought it was weird for images to be created at say the 2240 size, but really, the image is only 1024. Take this post for instance: https://www.oddbird.net/2019/04/09/vueconf/

That header image is 1024px but there is a version created that calls itself vueconf-2240.jpg. It will always stretch to fill the width of the page so it is kinda hard to see what is happening.

That said, I don't know if it is actually trying to create an image that is 2240, or if the filename change is saying, hey, show this image when the browser is at the "2240 size" which is actually half that for retina. So when my screen is around 640 px wide (or under), then it is showing me the 1280 (640 * 2) but when I go slightly above that, it will grab the 2240 image file.

I'm thinking that maybe the site needs to point to the same path pattern always, so if an image isn't large enough like the vueconf one, we still have to fake it or the link would be broken? I'm not exactly sure if this is something that would cause the timeout or delay or perhaps just take longer but I thought it was worth pointing out.

Screen Shot 2021-07-08 at 11 31 28 PM

@dvdherron
Copy link
Contributor Author

@dvdherron so I've been thinking about this since I reviewed Sondra's PR that removed the bg from images a week or two ago... I thought it was weird for images to be created at say the 2240 size, but really, the image is only 1024. Take this post for instance: https://www.oddbird.net/2019/04/09/vueconf/

That header image is 1024px but there is a version created that calls itself vueconf-2240.jpg. It will always stretch to fill the width of the page so it is kinda hard to see what is happening.

That said, I don't know if it is actually trying to create an image that is 2240, or if the filename change is saying, hey, show this image when the browser is at the "2240 size" which is actually half that for retina. So when my screen is around 640 px wide (or under), then it is showing me the 1280 (640 * 2) but when I go slightly above that, it will grab the 2240 image file.

@stacyk I think that is what's happening. That size you see when you inspect the image is just it's raw size in the browser. But if you look in the network panel it'll show whatever file the browser as chosen to be the best resolution at the current browser width.

I guess I'm a little unclear how one tool (gulp responsive images) makes all those sizes (480, 960, 1280, 2240) of the same images and one (11ty plugin) decides those same files aren't good enough to make at the bigger sizes.

I think this current setup could work for now. We could cut the file sizes down to just three (replace 2240 with 1600) if that looks okay. And replace images that look too low quality?

I'm thinking that maybe the site needs to point to the same path pattern always, so if an image isn't large enough like the vueconf one, we still have to fake it or the link would be broken? I'm not exactly sure if this is something that would cause the timeout or delay or perhaps just take longer but I thought it was worth pointing out.

I think the timeout issue isn't a problem now since we're no longer making .avif files.

@dvdherron dvdherron requested a review from jgerigmeyer July 12, 2021 16:39
Copy link
Member

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

I think this generally looks good. I do see that some images have more jpeg blurriness/artifacts than before. Is that because of a difference in quality settings? Does it help to bump those up a bit, or no?

I don't see any massive offenders off-hand, and I think this branch has some other advantages, so I'm happy to merge and keep improving in another story if we want.

@stacyk
Copy link
Member

stacyk commented Jul 12, 2021

@dvdherron @mirisuzanne I think that the images might appear fuzzier only if they are used in a full width or forced width situation and they were under the image crop size. for instance, this page: https://deploy-preview-144--oddleventy.netlify.app/2021/04/05/containerqueries/ vs https://www.oddbird.net/2021/04/05/containerqueries/ ... the hero image is originally 1408px wide so on the live site it is using that since the target size was 2240. On the deploy preview, the image's "intrinsic size" is 960 since the original image isn't big enough to be scaled/cropped at 1600 and I think we dropped the 1280 in favor of having only 3 files sizes. That is exactly how we want it I would think and it works a bit different than the current live site and I think that is good. We could certainly bump that 960px image size up a bit, but I think it is more important to always create a 1600px version of the hero and also document that somewhere so we all know.

Screen Shot 2021-07-12 at 4 46 16 PM

If you look at this page the deploy preview (bottom) is actually more crisp than the live site: https://deploy-preview-144--oddleventy.netlify.app/talks/wordpress-block-enhancements/
vs https://www.oddbird.net/talks/wordpress-block-enhancements/

I believe the original image size is 1600px on that one, so the live site is displaying the 1280px version of it and the deploy preview shows the 1600px version of it. This is a comparison with my browser at 1828px wide:

image

Lastly, the images in the CQ Quick start blog posts are originally 660px wide, so the deploy preview is scaling them down to the highest size it can, which is 480px, then the CSS tells it it must span to be 100% of the column. I think we will want to add some classes in there to make sure the images aren't forced full width (they aren't if we don't use figure and instead use just img, or if we add size-half but that isn't clear. Plus, figure I think is the only once with the caption so we may want to add some flexibility for that one. ALSO, we will probably always want to make sure to add images that are 1600 so the process can scale/crop and use the size it needs to. Not much we can do about past posts unless we manually go in and create bigger images.

Screen Shot 2021-07-12 at 5 01 07 PM

@dvdherron dvdherron marked this pull request as ready for review July 13, 2021 10:21
@dvdherron
Copy link
Contributor Author

dvdherron commented Jul 13, 2021

Per our standup conversation:

  • this PR is okay to merged (pending @jgerigmeyer's review?)
  • @stacyk suggested we make new cards for
    1. Adjusting the embed.figure macro so that it includes options for not forcing images to fill up 100% of the space.
    2. Documenting somewhere that new images used on the site, especially those that need to display wide, need to be at least 1600px.

A new card probably needs to be made for a workaround for the wrapping paragraph tags too. I'll add that. (There's already a card for this) And @stacyk if you don't get to adding cards above I can add those too.

* main:
  Combined deps upgrade
Copy link
Member

@stacyk stacyk left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I'll probably add some test coverage if @dvdherron doesn't want to do that.

I pretty much always see slightly blurrier images on this PR (even the one @stacyk said is clearer for her: https://deploy-preview-144--oddleventy.netlify.app/talks/wordpress-block-enhancements/) -- but I don't think it's too bad.

jest.config.js Outdated Show resolved Hide resolved
@dvdherron
Copy link
Contributor Author

@dvdherron I think this was overlooked? RSS and og:image tags were breaking.

Thanks for catching that. These are pointing to the right places now.

@jgerigmeyer jgerigmeyer merged commit c47ec1a into main Jul 14, 2021
@jgerigmeyer jgerigmeyer deleted the responsive-images branch July 14, 2021 16:24
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.

5 participants