lazy load images using lazyload-rails gem #8043
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8043 +/- ##
==========================================
+ Coverage 82.34% 82.49% +0.15%
==========================================
Files 99 99
Lines 5737 5737
==========================================
+ Hits 4724 4733 +9
+ Misses 1013 1004 -9
|
| @@ -0,0 +1,254 @@ | |||
| /*! | |||
There was a problem hiding this comment.
Ah! Is this something we could include via yarn instead, pegged to a specific version? Thank you!!!
There was a problem hiding this comment.
Yeah done! I still have to add more routes and refine this so no review required right now, I will ping when done 😅
|
@jywarren just checking in to ask if for the placeholder should we have like an animated spinner my only concern is that the size of it should be small. Or we can leave it at the gray placeholder as well 😅 Thought? ✌️ |
|
@Tlazypanda great job finding the gem. I have a few concerns with it though...its seems that its not frequently updated(last updated 14 months ago), it is crucial we look at this as it could hinder our future updates..also @emilyashley guidelines on vetting libraries #8019 ...I am not discarding it just giving you smth to think about/ consider . Thanks |
|
Hey @cesswairimu I totally understand your concerns over this 😅 and I actually did check it out against the vetting points by @emilyashley so the thing is that although this gem is not updated as frequently, the current implementation is something that won't need updates in the sense it is sufficient in itself. It is also the only implementation for lazy-loading in rails applications. And after doing a lot of searches, I could only find this tool being mentioned in blogs to speed up rails apps. The documentation is sufficient, there aren't any rollbacks in the commits as such and the license is MIT. Since, it is the only option available we might just have to go with it |
|
cool 👍 |
Tlazypanda
changed the title
|
Hey @cesswairimu @jywarren I have added some more routes now can you please review this? 😅 |
|
Hey, i just want to say the discussion and priorities of maintainability and the vetting and everything that happened in this PR is AWESOME 🙌🙌🙌🙌 ❤️❤️❤️ |
|
Reading y'all vet this just made me so happpppy! Good work. Excellent, nuanced points <3 |
| @@ -4,9 +4,9 @@ | |||
| <%= render partial: 'dashboard/node_moderate', locals: { node: node } %> | |||
|
|
|||
| <% if node.main_image %> | |||
| <a class="img" href="<%= node.path %>"><img src="<%= node.main_image.path(:default) %>" style="width:100%;" /></a> | |||
| <a class="img" href="<%= node.path %>"><%= image_tag(node.main_image.path(:default), style:'width:100%;') %></a> | |||
There was a problem hiding this comment.
Hi @Tlazypanda - this seems to have disrupted the loading of some images which are "blob" type - see for example the admittedly odd ones in this view:
https://publiclab.org/embed/grid/grid:infragram-upload
See how these actually use a dataurl as the image? I know its weird but I see the image_tag method is replacing the filename with blob - can we include a workaround, perhaps, if the image path is a data url?
This is a pretty weird workflow... i feel like maybe I'm missing something!
Also, I'm trying to fix the somewhat related assets on that template (for use in embedded content), here:
36c54a2
There was a problem hiding this comment.
Hey @jywarren I was thinking of writing a conditional rendering for this but am having some difficulty in defining the condition I was thinking of something like:-
<% elsif current_user.photo.url.start_with?("data:image") %> but doesn't seem to work... My understanding of the models may not be at point here can you help me out 😅
There was a problem hiding this comment.
Yeah, hmm. Are we mixing up blobs with data_urls? This is a bit mysterious! Uh, is it possible that the lazy load gem is converting a data-url into a blob?
AH! I remember, i wrote some really obscure code here. Let's see...
OMG YES -- #6931 and #6930 -- deep mysterious code! But yes, it has the data-uri-to-blob method there. That should help us!
There was a problem hiding this comment.
Should we consider just turning off lazy loading for this one instance in order to recover the functionality? @TildaDares @Manasa2850 @17sushmita are any of you interested in this particular bug? Thank you all!
There was a problem hiding this comment.
I was looking into this and was playing around with some modifications on the unstable but I'm not sure how to reproduce this on my local. Would love to get any help regarding this.
There was a problem hiding this comment.
Unfortunately I'm not sure how to reproduce... I'm not sure how the blob images are saved! Can we dig into the raw html shown on unstable to try to learn more?
There was a problem hiding this comment.
My best guess here is that we should just turn off lazy loading for this one type of image.
There was a problem hiding this comment.
OK this file is for the old dashboard! So ignore this particular file, we'll revert the one specifically for the new card-style display of nodes, below, i'll leave a comment.
| @@ -1,8 +1,8 @@ | |||
| <div class="card"> | |||
| <% if node.main_image %> | |||
| <a class="card-img-top img" style="overflow: hidden; height:10em;"<% if @widget %>target="_blank"<% end %> href="<%= node.path %>"><img src="<%= node.main_image.path(:default) %>" style="width:100%;"/></a> | |||
There was a problem hiding this comment.
So let's revert this entire file, only. That should solve it!
Fixes #7919
Lazy loaded images by using lazyload-rails gem. Converted the img tags into image_tag helpers for the gem to function properly.
rake test@publiclab/reviewersfor help, in a comment belowGIFs
Thanks!