Skip to content

Conversation

@adrianaixba
Copy link
Collaborator

Second checkbox of #11642

Makes naturalWidth/Height property optional. Allows for these properties to be set only when sure.

@adrianaixba adrianaixba requested a review from a team as a code owner November 23, 2020 23:02
@adrianaixba adrianaixba requested review from adamraine and removed request for a team November 23, 2020 23:02
@google-cla google-cla bot added the cla: yes label Nov 23, 2020
@adrianaixba adrianaixba changed the title core(ImageElements): Make naturalWidth/Height property optional core(imageElements): make naturalWidth/Height property optional Nov 23, 2020
@adrianaixba adrianaixba changed the title core(imageElements): make naturalWidth/Height property optional core(image-elements): make naturalWidth/Height property optional Nov 23, 2020
assert.equal(auditResult.items.length, 0);
assert.equal(auditResult.warnings.length, 1);
// Images with non valid naturalWidth or naturalHeight are ignored.
assert.equal(auditResult.warnings.length, 0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we intend to skip the images with invalid width/height, would we still need the warnings property of the uses-responsive-images.js audit? Is it reasonable to remove it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or include the invalid images to the warnings object? 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

If the warning ever triggers it would be a bug, so I would be in favor of removing the warning but still capturing in sentry.

image-aspect-ratio has a similar warning.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure there's ever been complete alignment on what we should warn vs. skip going back several years, but I'm also +1 to removing the warnings here :)

I'm of the mindset that the warning information in cases like these is not actionable and of little value to the user, so given the current prominence we provide warnings I am against keeping them as warnings.

I see the value in something that lets a developer know that something weird happened during Lighthouse run (like the errors/warnings sidebar design) but until that day comes, I think it's mostly just noise and should be avoided unless there's actually an action for the developer to take (like the crossorigin warnings we have).

assert.equal(auditResult.items.length, 0);
assert.equal(auditResult.warnings.length, 1);
// Images with non valid naturalWidth or naturalHeight are ignored.
assert.equal(auditResult.warnings.length, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the warning ever triggers it would be a bug, so I would be in favor of removing the warning but still capturing in sentry.

image-aspect-ratio has a similar warning.

Copy link
Contributor

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Overall LGTM

Copy link
Contributor

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

LGTM pt2

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.

6 participants