Skip to content

new_audit: add unsized-images to experimental config#11115

Merged
connorjclark merged 50 commits into
masterfrom
unsized-images
Jul 29, 2020
Merged

new_audit: add unsized-images to experimental config#11115
connorjclark merged 50 commits into
masterfrom
unsized-images

Conversation

@lemcardenas

@lemcardenas lemcardenas commented Jul 17, 2020

Copy link
Copy Markdown
Contributor

Summary

This PR is a first iteration in creating a new audit that confirms users' images are explicitly sized; ultimately preventing layout shift and improving CLS (cumulative layout shift).

Setting explicit sizes for images is an effective way to reduce CLS since images without sizes are a leading contributor to CLS.

Design and detail about explicit sizing on image elements can be found here.
Design and discussion about explicit sizing on various HTML elements can be found here.

This audit is set to remain in experimental config until CSS sizing is included in a future image-elements gatherer PR.

Related Issues/PRs

#10085

unsized-images-failing-3 0
unsized-images-passing-3 0

@lemcardenas lemcardenas reopened this Jul 17, 2020
@lemcardenas lemcardenas changed the title Unsized images new_audit: add sized-images audit Jul 17, 2020

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this looks awesome @lemcardenas! one of the cleanest "draft" PRs I've ever seen 😄

a PR overview with references to what stage this is and what planned followups should be expected (maybe linking to the doc?) would be awesome. I'm thinking we should land this in the experimental-config.js rather than default-config.js until we can update it to support CSS images, but maybe you already discussed an alternative plan/timeline I missed?

Comment thread lighthouse-core/gather/gatherers/image-elements.js Outdated
Comment thread lighthouse-core/scripts/i18n/collect-strings.js Outdated
Comment thread types/artifacts.d.ts Outdated
Comment thread lighthouse-core/audits/sized-images.js Outdated
@lemcardenas

Copy link
Copy Markdown
Contributor Author

a PR overview with references to what stage this is and what planned followups should be expected (maybe linking to the doc?) would be awesome.

Yes! I am planning on adding all of that, I didn't expect feedback on a draft PR 😅

I'm thinking we should land this in the experimental-config.js rather than default-config.js until we can update it to support CSS images, but maybe you already discussed an alternative plan/timeline I missed?

I think placing it in experimental-config is a good idea! I'll switch it over to that today

@lemcardenas lemcardenas marked this pull request as ready for review July 27, 2020 16:41
Comment thread lighthouse-core/audits/unsized-images.js Outdated
Comment thread lighthouse-core/audits/unsized-images.js Outdated
Comment thread lighthouse-core/audits/unsized-images.js Outdated
Comment thread lighthouse-core/audits/unsized-images.js Outdated
Comment thread lighthouse-core/config/experimental-config.js Outdated
Comment thread lighthouse-core/audits/unsized-images.js Outdated
Comment thread lighthouse-core/audits/unsized-images.js Outdated
Comment thread lighthouse-core/audits/unsized-images.js Outdated

@connorjclark connorjclark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

only a couple of minor style nits from me. after that it'll be LGTM

@vercel vercel Bot temporarily deployed to Preview July 29, 2020 00:27 Inactive
@connorjclark connorjclark merged commit c75f1ba into master Jul 29, 2020
@connorjclark connorjclark deleted the unsized-images branch July 29, 2020 01:16
@lemcardenas lemcardenas changed the title new_audit: add sized-images to experimental config new_audit: add unsized-images to experimental config Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants