Introduce responsive-images to teams page

[simonihmig]

I have worked over the past weeks on a complete overhaul of https://github.com/kaliber5/ember-responsive-image, which brings a number of exciting new features, like support for next-gen image formats (WebP, AVIF), LQIP, and much more. See the Readme for more details. Introducing this addon has already been discussed in #184, now with the improved feature set this makes even more sense IMO. I wanted to test this out in some real apps before releasing a stable v2, so here we go...

I chose to start with the teams page, as this was most straightforward to start with (all images have a fixed, static size). But if you folks agree on this, we can roll this out step by step to other pages as well...

This makes sure that the images files all have a width of 100px (200px for retina screens), will be served as AVIF, WebP or JPEg (whatever the client supports), and have a nice Blurhash based Low Quality Image Placeholder (LQIP) while (lazily) loading, and few more improvements, see the addon's Readme.

This should improve a bunch of image-related Lighthouse metrics (I saw that you have LHCI setup, which is nice, but it seems only the entry page is measured).

Before:

After:

(no idea what that warning means!?)

Here's a little screen capture how the lazy loading with LQIP will look like (simulating slow 3G). You can see that on my machine (MBP Chrome), AVIF files with 200px (2x) are lazily loaded...

Kapture.2021-02-20.at.13.22.02.mp4

/cc @mansona

[ijlee2]

Hi, @simonihmig. Thank you for working on updating ember-responsive-image and adding features.

I think it's a good idea to try out the addon in Teams page. I have a few questions before we merge the pull request:

• Percy snapshot seems to indicate that there's no guarantee which images have started or finished loading when the snapshot is taken.

  

  Is it possible to pursue one of these approaches?

  • Update the configuration in ember-cli-build.js to load images as usual in testing environment (perhaps pass an empty array to the include option in testing)
  • Have the addon tell us when the image has finished loading (perhaps expose the isLoaded tracked property as a data attribute, or provide a test helper?)
  • Some other solution

• Can you update the addon to import htmlSafe from @ember/template instead of @ember/string? The latter syntax is planned for deprecation.

  • https://github.com/kaliber5/ember-responsive-image/blob/v2.0.0-rc.1/addon/helpers/responsive-image-resolve.ts#L2
  • https://deprecations.emberjs.com/v3.x#toc_ember-string-htmlsafe-ishtmlsafe

• Could you address linting errors (missing commas)?

[simonihmig]

Percy snapshot seems to indicate that there's no guarantee which images have started or finished loading when the snapshot is taken.

Hm, this is weird.

If you look closely at percy's snapshot, you will see the "broken image" icon in the top left corner (it's clipped due to the border-radius of the parent element). This is indicating that the images could not be loaded, rather than that it is still loading! 🤔

Not sure what's going on there. I can see no issues when manually visiting the page in Firefox (seems that's what percy is using here). I don't know percy really, can you somehow see details, like the console output, or network failures (404s)?

Update the configuration in ember-cli-build.js to load images as usual in testing environment (perhaps pass an empty array to the include option in testing)

That's not possible. Using an image with <ResponsiveImage/> that is not processed through the addon throws an assertion. And even if it worked, we would be testing a different thing than what we would deploy to production, right?

Have the addon tell us when the image has finished loading (perhaps expose the isLoaded tracked property as a data attribute, or provide a test helper?)

It is not really doing much here. The load event is just used to remove the blurry background. Other than that, it is behaving just like a static <img ... loading="lazy">, so it's the browser who does the lazy loading natively. If it worked reliably before (regarding timing), than I see no reason why this could have changed here.

But again, as said above, it seems more than something is failing to load somehow...

Can you update the addon to import htmlSafe from @ember/template instead of @ember/string? The latter syntax is planned for deprecation.

Oh, thanks, didn't know this. Will do as part of the next release!

Could you address linting errors (missing commas)?

Done.

[ijlee2]

@simonihmig Thank you for addressing my feedback quickly. I will have a look at what's going on with the Percy snapshot tomorrow, see if there's something that we can do from this repo's side.

[simonihmig]

Just found another issue: the LQIP is not rendered in FastBoot (as it uses a modifier), which is where you want it the most. 🙈 Good to have some real world testing, guess this need still a bit more work...

[ijlee2]

@simonihmig Okay, let us know when the pull request is ready for review again. I will convert the PR to draft mode in the meantime.

[simonihmig]

@ijlee2 I worked over the weekend on the FastBoot related parts (simonihmig/responsive-image#162 and simonihmig/responsive-image#172), which should fix that issue.

The other issue regarding Percy remains though, and I am not sure how to proceed here, as I have no experience with percy. Do you know it well enough, or someone else on the team? E.g. can we see browser logs somewhere, or run it locally?

I just looked at the netlify preview deployment. When starting from another page (say index), throttling bandwidth (in dev tools) to slow 3G, and then navigation to the teams page, you will see the blurry placeholder rendered correctly:

So the broken image icon you see in those percy snapshots is not present. Which indicates that something in percy is behaving differently (than locally or on netlify).

[ijlee2]

@simonihmig Yep, I can help out with Percy. Not sure if there's a log that Percy generates that we can view, but it is possible to run Percy locally when you run tests with ember t. I will reach out to you on Discord with more information.

[simonihmig]

@ijlee2 thanks for the one-on-one today!

So I was still not able to reproduce this locally, but at least I was able to get closer to the root issue. It seems that Firefox is trying to load the AVIF files. Only the very latest FF version has actually AVIF support, though I had to enable it using an about:config flag - seems it is already active somehow when running percy in CI.

And something goes wrong there. I disabled AVIF to test this out, and indeed the snapshot then looks as expected! And as I mentioned already, it is definitely not related to any async network timing issues. Now sure why this happens only in CI, not locally.

One possible explanation could be that the MIME-type for .avif files is not correctly configured for the local static file server that ember-cli uses, as AVIF files are served as application/octet-stream. This is also visible in the percy logs: https://github.com/ember-learn/ember-website/runs/1960513880?check_suite_focus=true#step:8:12116

I did try this out locally though - running the same serve-static express middleware that ember-cli seems to use for ember test - but again could not reproduce the issue in Firefox. 😕

For now I only enable AVIF file generation in a production build. This fixes the percy issue. It still reports a visual change, but this time there are just some little changed nuances in the images, due to the modified images (scaled properly, and using WebP).

I'll keep this in draft mode for now, as I still have to release a new addon version with the latest FastBoot optimizations.

[ijlee2]

@simonihmig Thank you for investigating what happened in the Percy snapshot. It was certainly an unexpected issue and I'm glad that you figured out what happened.

Yep, minor pixel differences due to a change in algorithm is not a problem. I'll attach a screenshot of Percy's debug log below for posterity.

[simonihmig]

Updated this to the just released stable version of the addon. This improves a bunch of things, some of which were mentioned above. The addon now also does not generate AVIF files by default anymore, as they increase the build time considerably (given enough (large) images), and browser support is still limited to Chrome. So the percy issue should hopefully not happen anymore, even without the previous workaround. We might enable AVIF later, as soon as this seams reasonable (faster AVIF processing is supposed to be coming to sharp).

Ready for review!

[simonihmig]

The percy failure is just some noise in the images again, but not sure what's wrong with the linting: also there is just one linting job defined, it shows up twice here: one passing, and one failing without any logs!? 😕

[ijlee2]

Looks good, I look forward to see what other things in the website can be improved with ember-responsive-image. I reran the CI, which removed the duplicate the lint check.