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

add image lazy loading using wagtail-lazyimages plugin #2081

Closed
wants to merge 6 commits into from

Conversation

HamzaIbnFarooq
Copy link
Contributor

@HamzaIbnFarooq HamzaIbnFarooq commented Jan 22, 2021

Pre-Flight checklist

  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

#2065

What's this PR do?

Uses wagtail-lazyimages plugin and customizes it in a wrapper template tag (wagtail_image_lazy_load) to achieve enhanced caching

NOTE:

  1. Signatory images should not use this feature as if a person opens the certificate and directly prints it, without getting it into the viewport, a blurry signature will be displayed in the pdf.
  2. Some image tag filters like "height-110|format-png" are not supported by Wagtail-lazyimages plugin so I couldn't use it in learning-techniques.html inorder to use it we need to change that filter to something like max-150x150|format-png
    I have changed this in this commit

@codecov-io
Copy link

codecov-io commented Jan 22, 2021

Codecov Report

Merging #2081 (528da62) into master (3aaef79) will increase coverage by 9.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2081      +/-   ##
==========================================
+ Coverage   88.00%   97.14%   +9.14%     
==========================================
  Files         328      157     -171     
  Lines       14759     4934    -9825     
  Branches     1030        0    -1030     
==========================================
- Hits        12988     4793    -8195     
+ Misses       1532      141    -1391     
+ Partials      239        0     -239     
Impacted Files Coverage Δ
static/js/components/forms/BulkEnrollmentForm.js 90.62% <0.00%> (-1.57%) ⬇️
cms/forms.py
mitxpro/permissions.py
sheets/deferral_request_api.py
authentication/views.py
courseware/api.py
b2b_ecommerce/conftest.py
ecommerce/models.py
courses/management/utils.py
mitxpro/templatetags/render_bundle.py
... and 161 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3aaef79...528da62. Read the comment docs.

@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 22, 2021 13:43 Inactive
@gsidebo gsidebo mentioned this pull request Jan 22, 2021
3 tasks
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 07:47 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 09:16 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2065-image_lazy_loading branch from d220bc0 to 4e9de87 Compare January 25, 2021 09:16
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 09:16 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2065-image_lazy_loading branch from 4e9de87 to 83d569c Compare January 25, 2021 09:31
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 09:32 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2065-image_lazy_loading branch from 83d569c to cdd30bf Compare January 25, 2021 09:36
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 09:36 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2065-image_lazy_loading branch from cdd30bf to e04889a Compare January 25, 2021 10:28
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 10:28 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2065-image_lazy_loading branch from e04889a to 972deeb Compare January 25, 2021 11:18
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 11:18 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2065-image_lazy_loading branch from 972deeb to c3001c4 Compare January 25, 2021 11:23
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 11:23 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2065-image_lazy_loading branch from c3001c4 to 720e83a Compare January 25, 2021 13:50
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 13:50 Inactive
@arslanashraf7
Copy link
Contributor

@HamzaIbnFarooq I was able to test it on /catalog page but while testing it on homepage i see below error, related to learning-techniques.html. You will have to add learning techniques section in Home Page to reproduce it, let me know if you'll need more info.

Exception Value: filter specs in 'image' tag may only contain A-Z, a-z, 0-9, dots, hyphens and underscores. (given filter: "height-110|format-png")

`

@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 14:12 Inactive
@HamzaIbnFarooq
Copy link
Contributor Author

HamzaIbnFarooq commented Jan 25, 2021

Exception Value: filter specs in 'image' tag may only contain A-Z, a-z, 0-9, dots, hyphens and underscores. (given filter: "height-110|format-png")

Thanks @arslanashraf7, for the review, that filter ("height-110|format-png") isn't supported by Wagtail-lazyimages plugin so I have removed its usage from https://github.com/mitodl/mitxpro/blob/master/cms/templates/partials/learning-techniques.html#L8

Updated:
I have updated the image tag in this commit

@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 14:51 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-2081 January 25, 2021 15:08 Inactive

<div class="cover-image">
<div class="cover-image-frame">
{% if courseware_page.thumbnail_image %}
<img src="{% image_version_url courseware_page.thumbnail_image "fill-275x155" %}" alt="{{ courseware_page.thumbnail_image.title }}" />
{% wagtail_image_lazy_load courseware_page.thumbnail_image fill-275x155 alt=courseware_page.thumbnail_image.title class="wagtail-lazy-load" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we wouldn't need to specify the same class for every one of these images. It would be nice to have a default class and the option to extend. A param like addedClass might make sense. Also, the class can just be called lazy instead of wagtail-lazy-load

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do agree with your suggestions but I am trying to keep it as simple as we can. The issue is with the template tag we are using, when we create a simple_tag @register.simple_tag() it works as a function to which you can pass values and do some **kwargs stuff but in our case, we have to use @register.tag(name="xyz") as the function have to call another wagtail's tag named image.
Both the tags have different parsing techniques. So adding a new attribute makes the whole process complicated and messy.
As far as changing the name of wagtail-lazy-load to lazy is concerned, I have updated that.

return ""

raw_img_tag = super().render(context)
parsed_content = BeautifulSoup(raw_img_tag, "lxml").img
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach is clean enough in terms of avoiding code repetition, and it's unfortunate that the library doesn't give us extension options, but I'm not sure I agree with this approach of generating the image tag with the library implementation, parsing it as XML, adding attributes, and casting it as a string again. I think I would prefer even a regex replace – after all, we're just tacking on the same ?v=<hash> to the end of those two attribute values. The XML parsing just seems like more work than we need to be doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

threshold: 0
};

const imgs = document.querySelectorAll(".wagtail-lazy-load");
Copy link
Contributor

Choose a reason for hiding this comment

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

Selector can be changed to img.lazy if you change that class name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@gsidebo
Copy link
Contributor

gsidebo commented Jan 27, 2021

@HamzaIbnFarooq Sorry, I should have looked into this in my earlier review, but I noticed an issue with this library. Wagtail generates images on the fly depending on how you render them. This caused major performance issues for us, so we introduced a change to cache those generated images in Redis (#2029). Unfortunately it looks like the _lazy versions of the images are not being added to the Redis cache, so those images must be generated each time they're requested. We still have issues with temporary outages or slow responses with these images even when they're in the Redis cache (#2019), so I'm worried that this will cause more problems than it solves. One option would be to fork this plugin repo and make sure it uses the cache, but that could be a lot of work. For now unfortunately I think the best option is to put this PR on hold. Again, sorry to put the brakes on this. Let me know what you think.

@HamzaIbnFarooq
Copy link
Contributor Author

HamzaIbnFarooq commented Jan 28, 2021

@gsidebo Thanks for the detailed review. I didn't know about the whole rendition process, I spent some time understanding it but as you said it's somewhat challenging to integrate into our scenario.
I will put this on-hold but besides that what if we add a new placeholder image through cms or as a static asset in our project and use that same image as _lazy for all the lazy loading images so that our purpose will be fulfilled?

@gsidebo
Copy link
Contributor

gsidebo commented Dec 1, 2021

Closing this. The library didn't suit our needs

@gsidebo gsidebo closed this Dec 1, 2021
@rhysyngsun rhysyngsun deleted the hamza/2065-image_lazy_loading branch August 11, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants