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

Closes #6530 #6531 #6532 #6534 #6535 #6552: Preload images markup #6559

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Apr 16, 2024

Description

Fixes #6530 #6531 #6532 #6534 #6535 #6552

This PR addresses several issues related to image preloading, specifically for images detected as Largest Contentful Paint (LCP) and Above The Fold (ATF) images. The changes ensure that various types of images are correctly preloaded and excluded from lazy loading to improve page load performance. Additionally, it fixes an issue where background images identified as the LCP were incorrectly labeled as 'img' in the database, they are now correctly labeled dynamically.

Documentation

User documentation

This update improves the page load performance by correctly preloading various types of images and excluding them from lazy loading when they are detected as Largest Contentful Paint (LCP) and Above The Fold (ATF) images. This means faster loading times for your website visitors.

Technical documentation

The code works by identifying images that are LCP and ATF, and then preloading these images to improve page load performance. It also correctly labels background images identified as LCP with the corrected type in the database. The changes are implemented across several files, including JavaScript for front-end changes and PHP for back-end changes.

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

New dependencies

n/a

Risks

n/a

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Documentation

  • I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
  • The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
  • I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitly.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • I listed the introduced external dependencies explicitly on the PR.
  • I validated the repo-specific guidelines from CONTRIBUTING.md.

@MathieuLamiot
Copy link
Contributor

@Miraeld Independently of the review, can you provide some info about test & validation of the content of this PR? I am thinking mostly: what templates has this been tested against to validate the new markups? Is there new tests passing on Rocket-E2e, or tests/templates that should be covered by this that are not passing?
Thanks

@Miraeld
Copy link
Contributor Author

Miraeld commented Apr 16, 2024

@MathieuLamiot ,
Integrations tests are here to make sure that if we get the right value within the database, the frontend will be correct.

In terms of detection and saving the data itself (role of the beacon-lcp.js), I've only tested manually, as we can't run JS in integrstion tests, sadly. I don't think there are e2e tests even tho some templates are there.

How I've been testing it:
Simply by using templates available in rocket-test-data repo.

Some are not available yet, so I've built my own on the same structure as we would do for e2e.
Then do the process manually to make sure everything is detected and saved properly.

@MathieuLamiot
Copy link
Contributor

In terms of detection and saving the data itself (role of the beacon-lcp.js), I've only tested manually, as we can't run JS in integrstion tests, sadly. I don't think there are e2e tests even tho some templates are there.

There should be E2E checking what is saved in DB after a visit on the page, and running on many templates from the QA team. @jeawhanlee should be able to show you, that's what he implemented last cooldown sprint.

@Miraeld
Copy link
Contributor Author

Miraeld commented Apr 16, 2024

@MathieuLamiot totally agreed,
I was planning to look at e2e, as in the same time it's part of the testing we need to do for 3.16.
It's just I did not want to hold this PR while doing the e2e as e2e is on another repo.

@remyperona remyperona requested a review from jeawhanlee April 16, 2024 21:39
Miraeld added 11 commits April 17, 2024 02:18
The type was hardcoded as `img`, now it becomes dynamic and set by the `lcp-beacon.js` script.
Preload `image-set` & `webkit-image-set`
Preload layered background images
Preload single background images
Preload responsive images
Preload pictures tags
@Miraeld Miraeld force-pushed the enhancement/preload-background-image branch from da215e9 to dcf4bd4 Compare April 17, 2024 01:21
Copy link

codacy-production bot commented Apr 17, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.03% (target: -0.10%) 10.64% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (38775e5) 36818 8212 22.30%
Head commit (c6a7c1e) 36851 (+33) 8209 (-3) 22.28% (-0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6559) 47 5 10.64%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

@MathieuLamiot
Copy link
Contributor

As discussed here, this PR is likely to miss the AC about ATF exclusion of Lazyload for elements that are not "img" (so video, picture, p, main, div)

@Miraeld
Copy link
Contributor Author

Miraeld commented Apr 18, 2024

Well I can add them here. I may be on the path to have a solution but need some optimization :)

@Miraeld Miraeld merged commit 41f5192 into feature/lcp-above-the-fold-optimization Apr 24, 2024
11 of 13 checks passed
@Miraeld Miraeld deleted the enhancement/preload-background-image branch April 24, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants