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: Skip images with data-skip-lazy attribute #14539

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Jan 31, 2020

Per a request from a hosting partner and a community member, add the ability to skip lazy loading of images that have the data-skip-lazy attribute.

Changes proposed in this Pull Request:

  • Do not lazy load images that have data-skip-lazy attribute

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This is a new feature for an existing module.

Testing instructions:

  • Checkout branch
  • Ensure Lazy Images is active
  • Create a post with an image
  • Modify image's HTML to include data-skip-lazy="1"
  • Publish post then load post
  • Check source of image, and ensure that it does not have the jetpack-lazy-image--handled class nor any other lazy attributes.

For example, there are two images in the source here. The top on was not lazy loaded but the bottom one was.

Screen Shot 2020-01-31 at 11 00 59 AM

Proposed changelog entry for your changes:

  • Update lazy images module to skip images with the data-skip-lazy attribute

@ebinnion ebinnion requested a review from a team January 31, 2020 16:21
@ebinnion ebinnion self-assigned this Jan 31, 2020
@ebinnion ebinnion changed the title Lazy Images: Skip images with data-skip-lazy attrib Lazy Images: Skip images with data-skip-lazy attribute Jan 31, 2020
@jetpackbot
Copy link

jetpackbot commented Jan 31, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: February 11, 2020.
Scheduled code freeze: February 4, 2020

Generated by 🚫 dangerJS against f4d0f59

@ebinnion ebinnion force-pushed the update/lazy-images-skip-lazy-data-attr branch from 9840d86 to f4d0f59 Compare January 31, 2020 16:26
@ebinnion ebinnion added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 31, 2020
@ebinnion ebinnion added this to the 8.2 milestone Jan 31, 2020
@jeherve
Copy link
Member

jeherve commented Jan 31, 2020

I'd have 2 questions about this:

  • Isn't the jetpack_lazy_images_skip_image_with_atttributes filter meant to do just that today?
  • We're already past the code freeze and in the Beta period for Jetpack 8.2; would it be acceptable to ship this in Jetpack 8.3 instead?

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 31, 2020
@ebinnion
Copy link
Contributor Author

ebinnion commented Jan 31, 2020

Isn't the jetpack_lazy_images_skip_image_with_atttributes filter meant to do just that today?

That's a good question. Yes, users and third-party developers could use that filter to exclude images based on an attribute. But, the purpose of adding this attribute is to standardize how users and third-party theme/developers can exclude images from being lazily loaded.

Other plugins use some combination of the skip-lazy class or the data-skip-lazy attribute. So, the goal is for all plugins to support both.

Of note, there's a core marketing push for this around this as well.

would it be acceptable to ship this in Jetpack 8.3 instead?

Yes sir.

@ebinnion ebinnion modified the milestones: 8.2, 8.3 Jan 31, 2020
@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 31, 2020
@@ -271,6 +271,34 @@ function get_dont_process_images_with_classes_data() {
);
}

/**
* @dataProvider get_dont_process_images_with_skip_lazy_data_attribute_data
Copy link
Member

Choose a reason for hiding this comment

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

TIL how to use @dataProvider! Thanks for adding the tests.

@dereksmart dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 12, 2020
@kraftbj kraftbj merged commit f583dfd into master Feb 13, 2020
@kraftbj kraftbj deleted the update/lazy-images-skip-lazy-data-attr branch February 13, 2020 05:32
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 13, 2020
jeherve added a commit that referenced this pull request Feb 25, 2020
jeherve added a commit that referenced this pull request Feb 25, 2020
* 8.3 release: changelog

* Changelog: add #14516

* Changelog: add #14574

* Bring in changes from 8.2.1 and 8.2.2

* Update stable version

* Bring in 8.2.3 changes

* Changelog: add #14714

* Changelog: add #14639

* Changelog: add #14678

* Changelog: add #14673

* Changelog: add #14687

* Changelog: add #14704

* Changelog: add #14702

* Changelog: add #14541

* Changelog: add #14657

* Changelog: add #14622

* Changelog: add #14582

* Changelog: add #14638

* Changelog: add #14633

* Changelog: add #14571

* Changelog: add #14592

* Changelog: add #14539

* Changelog: add #14514

* Changelog: add #14643

* Changelog: add #14494

* Changelog: add #13739

* Changelog: add #14707

* Changelog: add #14736

* Changelog: add #14706

* Changelog: add #14730

* Changelog: add #14685

* Changelog: add #14727

* Changelog: add #14711

* Changelog: add #14742

* Changelog: add #14746

* Changelog: add #14725

* Changelog: add #13999

* Changelog: add #14740

* Changelog: add #14759

* Changelog: add #14703

* Changelog: add #14753

* Changelog: add #14754

* Changelog: add #14645

* Cahngelog: add #14599
nicokaiser added a commit to nicokaiser/lazy-images-without-jetpack that referenced this pull request Mar 31, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants