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

Lazy Images: Allow filtering out images via class and attributes #8787

Merged
merged 3 commits into from
Feb 13, 2018

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Feb 8, 2018

Closes #8785
Closes #8641

In #8641, a user requests the ability to skip processing images with a given class.

In #8745, a user reports a compatibility issue with another plugin where we shouldn't be processing an image so that it's lazy. After looking at the plugin's code, I don't see any specific classes that we can hook on.

These two issues go pretty well together. 😄

This PR will begin skipping images where the skip-lazy class is applied. Further, a user can filter jetpack_lazy_images_blacklisted_classes to add or remove classes to the list of classes that we'll skip.

I've also added a filter that will allow skipping an image via its attributes. The jetpack_lazy_images_skip_image_with_atttributes receives an array of the current image's attributes as its second argument. Filters can hook on this, examine the attributes, and decide whether to skip the image or not.

These two methods of skipping images should provide a good amount of flexibility for plugins and themes.

To test:

  • Run tests phpunit --testsuite=lazy-images
  • Ensure lazy images is on
  • Install and activate gazette theme
  • Go to customizer, the "Featured Content" section, then select a tag to show featured posts
  • Ensure that some posts have featured images and the tag from the previous step
  • Load frontend and ensure that the featured image thumbnails up to are displaying correctly

Changelog entry

  • We now allow lazy images to skip images with the skip-lazy css class or any give class of your choice by using the jetpack_lazy_images_blacklisted_classes filter.

@ebinnion ebinnion added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Lazy Images labels Feb 8, 2018
@ebinnion ebinnion added this to the 5.9 milestone Feb 8, 2018
@ebinnion ebinnion self-assigned this Feb 8, 2018
@ebinnion ebinnion requested a review from a team as a code owner February 8, 2018 18:46
@ebinnion ebinnion force-pushed the fix/lazy-images-compat-shortcode-bakery branch from f809c6b to 3de68e9 Compare February 8, 2018 18:46
@oskosk
Copy link
Contributor

oskosk commented Feb 9, 2018

Tested. Unfortunately I haven't noticed any difference in the way the featured images load on the home page with Lazy Images turned on. My comprison was against master and against this PR.

But I do notice some difference on the individual post view. I'm testing again and getting some screencaps

@ebinnion
Copy link
Contributor Author

ebinnion commented Feb 9, 2018

Tested. Unfortunately I haven't noticed any difference in the way the featured images load on the home page with Lazy Images turned on. My comprison was against master and against this PR.

Is this with the Gazette theme? Ideally, you wouldn't see differences with the featured content that shows at the very top of the theme since we'd be skipping those images.

@ebinnion
Copy link
Contributor Author

Hey @oskosk – can you provide a bit more detail? You mentioned some screenshots in your last comment.

Copy link
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.

LGTM

@oskosk
Copy link
Contributor

oskosk commented Feb 13, 2018

Is this with the Gazette theme? Ideally, you wouldn't see differences with the featured content that shows at the very top of the theme since we'd be skipping those images.

So, following the testing instructions is that I noticed no difference on the homepage. Indeed with the Gazette theme.

The screenshots I mentioned are related to Lazy images and gazette but on the single post view.

2

That super high block that appears on the single post view is seen when Lazy Images is turned on, both on latest stable and in this PR.

Copy link
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.

As explained in slack, seeing no differences in the homepage for gazette implies that this changeset works fine. LGTM!

@gravityrail gravityrail merged commit 7c3d69b into master Feb 13, 2018
@Viper007Bond Viper007Bond deleted the fix/lazy-images-compat-shortcode-bakery branch February 13, 2018 19:28
oskosk added a commit that referenced this pull request Feb 27, 2018
oskosk added a commit that referenced this pull request Feb 27, 2018
* update changelog.txt

* Update readme.txt with scaffolding for 5.9 changelog and release draft shortlink

* Add changelog entry for #8243

* Add changelog entry for #8296

* Add changelog entry for #8367

* Add changelog entry for #8686

* Add changelog entry for #8707

* Add changelog entry for #8709 and #8714

* Add changelog entry for #8729

* Add changelog entry for #8777

* Add changelog entry for #8780

* Add changelog entry for #8786

* Add changelog entry for #8787

* Add changelog entry for #8801 #8805 #8832 #8865 and #8804

* Add changelog entry for #8817

* Add changelog entry for #8822

* Add changelog entry for #8823

* Add changelog entry for #8829

* Add changelog entry for #8834

* move some items to major enhancements

* Add changelog entry for #8836

* Add changelog entry for #8839

* Add changelog entry for #8861

* Add changelog entry for #8862

* Add changelog entry for #8863

* Add changelog entry for #8866

* Add changelog entry for #8870

* Add changelog entry for #8874

* Add changelog entry for #8875

* Add changelog entry for #8881

* Add changelog entry for #8890

* Add changelog entry for #8911

* Add changelog entry for #8927

* Add changelog entry for #8931

* Add changelog entry for #8933

* Add changelog entry for #8930

* fix wording

* typo

* minor fixes

* replace partner scripts for Jetpack Start in changelog entry

* Update to-test.md

* Update to-test.md

* minor style fixes to to-test.md

* minor style fixes to to-test.md

* minor fixes on to-test.md

* Add changelog entry for #8868

* Add changelog entry for #8844

* Add changelog entry for #8664

* Add changelog entry for #8935

* Add changelog entry for #8425

* Add changelog entry for #8625
@boiangeorgiev
Copy link

It's funny how half an year later no one noticed the typo in one of these filters.
It should be jetpack_lazy_images_skip_image_with_attributes and not jetpack_lazy_images_skip_image_with_atttributes. Apparently this filter is not used very often.

@ebinnion
Copy link
Contributor Author

ebinnion commented Aug 8, 2018

Hi @boiangeorgiev – Thank you for the report. I have created #10002 to address the typo.

@kraftbj kraftbj removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Lazy Images [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it Unit Tests
Projects
None yet
6 participants