PWA: Adds lazy image loading module #8093

ebinnion · 2017-11-02T14:02:22Z

Will help speed by minimizing the number of images we download when the page is first viewed.

This plugin is largely based on the Lazy Load plugin, but has been updated to make use of IntersectionObserver. IntersectionObserver does not have great browser support. But, we are using a polyfill that will add the necessary functionality.

I have several questions, which are listed in this comment, mostly around attribution and how we should load the polyfill.

Besides that, this PR should be ready for some review and testing.

To test:

Checkout PR
Load admin page
Go to $site.com/wp-admin/admin.php?page=jetpack_modules and enable the Lazy Images module
Go to front-end of site, and ensure that images get loaded as you scroll down the page. Tip: you can filter the network panel to only show img requests
Ensure that there are no JS errors
Test various browsers

gravityrail · 2017-11-02T15:48:23Z

modules/lazy-images/assets/lazy-images.js

This is no longer a TODO

ebinnion · 2017-11-03T19:50:41Z

At this point, things are functional in IE11 and Edge as well as Chrome.

Todos:

Add attribution to VIP team, @mjangda, and others
Integrate with infinite scroll. Currently, the script only fire once. We also need to fire each time posts are added to the dom.
Add conflicting plugins

ebinnion · 2017-11-03T20:48:45Z

@mjangda mentioned that we should consider pulling updates from here:

Automattic/lazy-load#10

ebinnion · 2017-11-06T19:55:50Z

At this point, we are functional back to IE9 as well as other browsers.

Known issues/questions at this point are:

How to properly attribute the IntersectionObserver polyfill and other code?
Is it OK to load the polyfill directly in lazy-images.js, or do we need to drop that in it's own file?
If we keep it in the same file, is it OK to leave it minified, or could that break the build process if we minify again when we eventually start minifying all module JS?
The Lazy Load repo above includes unit tests. We should probably port those over and perhaps add some more.

Note: I tried very hard to not have jQuery as a dependency here. But, since the post-load event is fired via jQuery, and jQuery uses a custom event system, it seems like we currently require jQuery, at a minimum to support Infinite Scroll.

ebinnion · 2017-11-06T20:17:06Z

modules/lazy-images/js/lazy-images.js

@@ -0,0 +1,134 @@
+/* globals IntersectionObserver, jQuery */
+
+/**


Does the license for the polyfill need to go here? Also, I'd like to keep the polyfill in this same file to minimize network requests. It bails early if it detects IntersectionObserver is in window, so it shouldn't cause issues. If we leave it here though, should me unminify it?

Just include the polyfill licence in Jetpack license, or somewhere like that. I don't think we need to serve it to the browser.

ebinnion · 2017-11-06T20:18:04Z

modules/lazy-images/lazy-images.php

+		 * @param string The URL to the placeholder image
+		 */
+		return apply_filters(
+			'lazyload_images_placeholder_image',


I decided to use the same filters as the Lazy Load plugin to allow for easy updating for VIP and other users. Should we use something else though?

I am torn. I guess we should keep them the same though. Sites switching from the existing plugin will experience brokenness/regressions otherwise, and people switching from other plugins won't care. Developers might think it's weird that we didn't prefix it with jetpack_, but only for about 5 seconds.

It would be good to leave a comment why we choose not to prefix it with jetpack_

gravityrail · 2017-11-06T20:54:16Z

could that break the build process if we minify again when we eventually start minifying all module JS

Minifying twice shouldn't break things. However, let's put this in un-minified so that people can read the code, and so that the unminified version can include its own license.

The Lazy Load repo above includes unit tests. We should probably port those over and perhaps add some more.

We can add them later if we want. I don't want to have this as a super-long-lived PR, and I think if we're confident this works it would be good to get it merged so we can get it in people's hands ASAP.

gravityrail · 2017-11-06T20:55:46Z

Is there any way to only load the intersection observer if it's necessary?

gravityrail

This is awesome

ebinnion · 2017-11-06T23:45:46Z

In talking to @oskosk, we realized that the Apache 2.0 license on the current IntersectionObserver polyfill is not compatible with GPLv2. So, I'll need to fix that before we can merge.

@gravityrail's did a search and found an MIT-licenses IntersectionObserver that I'll test tomorrow: https://github.com/que-etc/intersection-observer-polyfill

ebinnion · 2017-11-07T18:55:26Z

I've now updated the IntersectionObserver polyfill to use the above-linked MIT project. This can now be reviewed again.

…le one

…nline the polyfill

…tpack

oskosk

LGTM. Tested well even in combination with the mentioned plugin. The issue when photon is active, remains, but it's a matter for another PR

* Changelog 5.6: create base for changelog. * Update changelog with 5.5.1 info. * Changelog: add #7930 and #8238 * Changelog: add #8076 * Changelog: add #8100 * Changelog: add #8117 * Changelog: add #8141 * Changelog: add #8143 * Changelog: add #8147 * Changelog: add #8149 * Changelog: add #8153 * Changelog: add #8173 * Changelog: add #8184 * Changelog: add #8196 * Changelog: add #8199 * Changelog: add #8093 * Changelog: add #8171 * Changelog: add #8182 * Changelog: add #8202, #8222 * Changelog: add #8228 * Changelog: add #8240 * Changelog: add #8251 * remove AL card change

ebinnion added PWA [Status] In Progress labels Nov 2, 2017

ebinnion added this to the 5.6 milestone Nov 2, 2017

ebinnion self-assigned this Nov 2, 2017

ebinnion requested a review from a team as a code owner November 2, 2017 14:02

gravityrail reviewed Nov 2, 2017

View reviewed changes

modules/lazy-images/assets/lazy-images.js Outdated

Copy link

Contributor

gravityrail Nov 2, 2017

Choose a reason for hiding this comment

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

This is no longer a TODO

ebinnion reacted with thumbs up emoji

ebinnion force-pushed the add/lazy-images branch 6 times, most recently from 0d74c6e to 3982c84 Compare November 3, 2017 18:26

ebinnion force-pushed the add/lazy-images branch from f141a80 to f523351 Compare November 6, 2017 19:33

ebinnion added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Nov 6, 2017

ebinnion commented Nov 6, 2017

View reviewed changes

gravityrail approved these changes Nov 6, 2017

View reviewed changes

gravityrail added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Nov 6, 2017

ebinnion added [Status] In Progress and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 6, 2017

ebinnion force-pushed the add/lazy-images branch from fa7622a to 3a9aaeb Compare November 7, 2017 18:53

ebinnion added 17 commits November 10, 2017 14:12

Lazy Images: Add lazy images module to improve load time

99c9d51

Lazy Images: Only apply srcset if not falsey; Do not apply fade-in class

6ef0a5a

Lazy Images: Do not lazy load wp admin bar avatar

af81a68

Lazy Images: First pass at upstream updates

f37939d

Lazy Images: Move back to IntersectionObserver

d1ca06f

Lazy Images: Adds doc for lazy images placeholder image

344b05e

Lazy Images: Remove unused version

92aaf80

Lazy Images: Add support for Infinite Scroll

ea2b0b3

Lazy Images: Add top conflicting plugins

8aab996

Lazy Images: Add attribution for basis of module

43d1271

Lazy Images: Load image when within 200px vertically of viewport

92db0ba

Lazy Images: Add unit tests

0c9812d

Lazy Images: Set sizes attribute if stored in attribute

220001f

Lazy Images: Update IntersectionObserver polyfill to a GPLv2 compatib…

774ccb4

…le one

Lazy Images: Remove ignore for intersection-observer.js now that we i…

ece6c0d

…nline the polyfill

Lazy Images: Add comment to clarify why filters not prefixed with _je…

3ef178b

…tpack

Lazy Images: Add missing semicolon to please linter

4b007f0

ebinnion force-pushed the add/lazy-images branch from 1aef1c6 to 4b007f0 Compare November 10, 2017 20:12

oskosk approved these changes Nov 10, 2017

View reviewed changes

oskosk merged commit 6b47600 into master Nov 10, 2017

oskosk deleted the add/lazy-images branch November 10, 2017 20:47

This was referenced Nov 18, 2017

Photon + Lazy Load doesn't load Retina versions #529

Closed

Lazy Load + Photon will fail to show some images when there is a need to scale #530

Closed

Photon: handle Rocket Lazy Load #1638

Closed

jeherve added [Feature] Lazy Images [Status] Has Changelog and removed [Status] Needs Review This PR is ready for review. labels Nov 24, 2017

jeherve added a commit that referenced this pull request Nov 24, 2017

Changelog: add #8093

93743de

ebinnion mentioned this pull request Feb 20, 2018

Lazy Images: Use W3C IntersectionObserver polyfill #8890

Merged

		@@ -0,0 +1,134 @@
		/* globals IntersectionObserver, jQuery */

		/**

PWA: Adds lazy image loading module #8093

PWA: Adds lazy image loading module #8093

Uh oh!

Conversation

ebinnion commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gravityrail Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

ebinnion commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebinnion commented Nov 3, 2017

Uh oh!

ebinnion commented Nov 6, 2017

Uh oh!

ebinnion Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

gravityrail Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

ebinnion Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

gravityrail Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

enejb Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

gravityrail commented Nov 6, 2017

Uh oh!

gravityrail commented Nov 6, 2017

Uh oh!

gravityrail left a comment

Choose a reason for hiding this comment

Uh oh!

ebinnion commented Nov 6, 2017

Uh oh!

ebinnion commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oskosk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ebinnion commented Nov 2, 2017 •

edited

Loading

ebinnion commented Nov 3, 2017 •

edited

Loading

ebinnion commented Nov 7, 2017 •

edited

Loading

oskosk left a comment •

edited

Loading