Skip to content

Lazy Images: Add jetpack-lazy-loaded-image event#9451

Merged
oskosk merged 3 commits intomasterfrom
update/lazy-images-loaded-action
May 1, 2018
Merged

Lazy Images: Add jetpack-lazy-loaded-image event#9451
oskosk merged 3 commits intomasterfrom
update/lazy-images-loaded-action

Conversation

@ebinnion
Copy link
Copy Markdown
Contributor

@ebinnion ebinnion commented May 1, 2018

In #8879 it was reported that outdented images were broken in cases where the CSS class was applied after the initial page loading. In TwentySixteen, this seems to be because when the initial 1x1 image loads, it wasn't meeting the condition that it be below the entry meta content.

The first part of a fix here is to fire an event when we lazy load an image, which will allow themes and plugins to better integrate with Lazy Images.

I wasn't able to reproduce the multiple network requests that @PatTheMav reported in that issue. Based on his testing though, it seems possible that the issue was because we were calling image.setAttribute() on the image in the dom. What I've tried to do in this PR is to clone the original image, update the attributes, and THEN replace the original image with the new one. I hope this fixes the issue. 🤞

To test:

  • Checkout branch
  • Ensure lazy images is turned on
  • Create a post with several paragraphs and images
  • In console add the following:
    jQuery( document.body ).on( 'jetpack-lazy-loaded-image', function( e ) {
    	console.log( e.target );
    } );
  • Ensure that images load as you scroll and that the images get logged in the console
  • Test the above in TwentySixteen and ensure that images are outdented after they are lazy loaded.

Here's an example of an outdented image in TwentySixteen:

screen shot 2018-04-30 at 7 59 59 pm

Here's the same image not outdented:

screen shot 2018-04-30 at 8 00 23 pm

@ebinnion ebinnion added this to the 6.2 milestone May 1, 2018
@ebinnion ebinnion self-assigned this May 1, 2018
@ebinnion ebinnion requested a review from a team as a code owner May 1, 2018 00:36
@matticbot
Copy link
Copy Markdown
Contributor

Created a new revision for this PR: D12609-code

@matticbot
Copy link
Copy Markdown
Contributor

Updated the revision for this PR: D12609-code

@ebinnion ebinnion added Bug When a feature is broken and / or not performing as intended Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review This PR is ready for review. [Feature] Lazy Images labels May 1, 2018
@PatTheMav
Copy link
Copy Markdown

PatTheMav commented May 1, 2018

@ebinnion Just updating the srcset attribute (and leaving the src attribute alone) could IMO fix most issues I reported - is that out of the question because you want to support lazy loading for very old browser versions that don't support srcset (it would also remove the need for <noscript> fallbacks for e.g. Google bots and older browsers as those just load the src non-lazily: https://caniuse.com/#feat=srcset)?

The Javascript hook for post-load events was my workaround for other themes as well 👍🏻.

@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented May 1, 2018

I went with this approach for now in the hopes that it might make the 6.1 release coming out this week. I like the srcset approach, but I'd want more testing before releasing it.

@PatTheMav
Copy link
Copy Markdown

@ebinnion Got it, makes total sense. Let me know if I can assist - my blog currently runs a slightly modified "Independent Publisher 2" Wordpress.com theme.

Copy link
Copy Markdown
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

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

Works great for me on twentysixteen. Confirmed issue and this patch fixes it.

Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@oskosk oskosk 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 May 1, 2018
@oskosk oskosk merged commit e48a59f into master May 1, 2018
@oskosk oskosk deleted the update/lazy-images-loaded-action branch May 1, 2018 17:01
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 1, 2018
@matticbot
Copy link
Copy Markdown
Contributor

Updated the revision for this PR: D12609-code

oskosk pushed a commit that referenced this pull request May 1, 2018
* Lazy Images: Add jetpack-lazy-loaded-image event

* Lazy Images: Provide outdent support for TwentySixteen

* Lazy Images: Bail early if no image; Only update attrs if we have them
@oskosk oskosk modified the milestones: 6.2, 6.1 May 1, 2018
@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented May 1, 2018

Ported to branch-6.1 in 6821756

@PatTheMav
Copy link
Copy Markdown

Can confirm, when adding jQuery( document.body ).on( 'jetpack-lazy-loaded-image', function () { jQuery( window ).trigger( 'resize' ); } ); to "independent-publisher-2", outdenting happens as intended.

The resize event handlers in many themes are wasteful to boot, but that's not Jetpack's concern.. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Lazy Images

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants