Skip to content

Conversation

@atcastle
Copy link

@atcastle atcastle commented Nov 3, 2024

Note: This PR is currently a draft and untested, because of a build issue that currently prevents tests from running in the repo. @jerzy-mankowski has said they will address it when they have time. I will revise this PR afterward with a set of passing E2E tests at that time.

This PR includes a draft of 2 new performance warnings for the React AdvancedImage element. Please see the accompanying issue for a full description of the rationale for these two warnings.

The warnings are:
warnOnOversizedImage: Logs a warning when an image is loaded, and the intrinsic size is greater than the rendered size by a substantial margin. That margin is defined in OVERSIZE_IMAGE_TOLERANCE and initially set to 500.
warnOnLazyLCP: Logs a warning when the PerformanceObserver API detects an image is a candidate for the LCP image but has been loaded with loading=“lazy”.

Both warnings only fire if the NODE_ENV variable is present (true in most React apps) and set to “development”. Both warnings can be silenced if the AdvancedImage element is given an attribute called “silence-warnings”.

@jerzy-mankowski
Copy link
Contributor

jerzy-mankowski commented Nov 12, 2024

@atcastle the build issue has been resolved 👍 just need to fix the typing issues

Copy link

@lishaduck lishaduck left a comment

Choose a reason for hiding this comment

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

Happened to notice some typos while vendoring the Cloudinary SDK.

`and may slow down page load. You may address this by supplying a height and width for the image, ` +
`or by using the responsive image plugin. ` +
`Rendered size: ${clientWidth}x${clientHeight}. Intrinsic size: ${naturalWidth}x${naturalHeight}. ` +
`This warning can be surpressed by adding the 'silence-warnings' attribute to AdvancedImage.`

Choose a reason for hiding this comment

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

Suggested change
`This warning can be surpressed by adding the 'silence-warnings' attribute to AdvancedImage.`
`This warning can be suppressed by adding the 'silence-warnings' attribute to AdvancedImage.`
`An image with URL ${imgRef.src} has ' loading="lazy"' and has also been detected to be a possible ` +
`LCP element (https://web.dev/lcp). This can have a significant negative impact on page loading performance. ` +
`To fix this issue, remove, 'loading="lazy"' from images which may render in the initial viewport. ` +
`This warning can be surpressed by adding the 'silence-warnings' attribute to AdvancedImage.`

Choose a reason for hiding this comment

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

Same

Suggested change
`This warning can be surpressed by adding the 'silence-warnings' attribute to AdvancedImage.`
`This warning can be suppressed by adding the 'silence-warnings' attribute to AdvancedImage.`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants