Check if image is fully loaded #79

aderaaij · 2017-12-16T14:52:13Z

This is a quick and dirty pull request to check if images have already loaded. This will not pass your tests yet, and doesn't yet take in account srcset either, but I wanted to get the ball rolling!

A few thoughts.

I've modified the markAsLoaded function to simply implement a waiting for images function, data-loaded is only added after the image is fully loaded. I can imagine that there might be circumstances where you want to do something as soon as the image is added, so maybe it's an idea to add another data-attribute especially for when the image is fully loaded.

I also made a solution for srcset but it isn't very elegant yet. It uses p-wait-for, which only adds a couple of bytes to the total. I didn't include this solution in my pull request yet, I'd like to discuss it first!

function markAsLoaded(element) {
  if (element.dataset.srcset) {
    pWaitFor(() => {
      if (element.currentSrc !== '') {
        return true
      }
      return false
    }).then(() => {
      const imgLoad = new Image()
      imgLoad.onload = () => {
        element.dataset.loaded = true
      }
      imgLoad.src = element.currentSrc
    })
  } else {
    const imgLoad = new Image()
    imgLoad.onload = () => {
      element.dataset.loaded = true
    }
    imgLoad.src = element.dataset.src
  }
}

jimblue · 2017-12-16T17:38:33Z

@aderaaij this could help don't you think?

https://github.com/marlospomin/turtle/blob/master/src/turtle.js

aderaaij · 2017-12-16T17:41:10Z

@jimblue Yeah it's done in a nice way, somewhat similar actually! Only it doesn't have solutions for srcset included.

jimblue · 2017-12-16T17:42:45Z

Indeed @aderaaij... does the actual PR work for background image on your side? I doesn't here

aderaaij · 2017-12-16T17:43:55Z

Good question! No it doesn't, I only check the normal src now. Background image shouldn't be too hard though. I'll have a check later on

jimblue · 2017-12-16T17:45:05Z

Allright. I'll work on this too. 😄

jimblue · 2017-12-16T18:02:34Z

I was thinking about a nice way to make the loading feeling really smooth:

observe all .lozad elements
preload image outside of intersection (for exemple: rootMargin * 2)
mark image as loaded when reaching intersection (rootMargin * 1)

The benefit is to preloaded images outside viewport but showing the animation when reaching viewport.

What do you think?

aderaaij · 2017-12-16T18:06:19Z

It's a good idea but I think that would be for users themselves to define. You can override the rootmargin and treshold when calling the script, so everyone can figure out what works smooth for themselves, as long as there is a part in there that tells when the image is fully loaded.

jimblue · 2017-12-16T18:33:22Z

That's true

aderaaij · 2017-12-16T19:22:04Z

Added support for background images

jimblue · 2017-12-17T20:26:29Z

Thanks @aderaaij!!!! I can't wait @ApoorvSaxena have a look to this PR 😝 !

jimblue · 2017-12-21T06:38:26Z

Hi @aderaaij! Do you now why checks have failed?

OlehDutchenko · 2017-12-21T08:14:57Z

@jimblue look at Travis log
https://travis-ci.org/ApoorvSaxena/lozad.js/builds/317439884?utm_source=github_status&utm_medium=notification

jimblue · 2017-12-21T08:40:07Z

oh ok just learn somethin! Thanks @dutchenkoOleg ... Just don't know how to fix this ^^

aderaaij · 2017-12-21T08:49:22Z

@jimblue in this pull-request I explained I didn't bother passing all the tests as I wanted to discuss this PR first. I don't know too much about tests, but they're failing because they're expecting an image to be returned right away, so they probably also need the 'loading' script included. Oh and there's one about SRC Set too 😅.

But to be honest I think @ApoorvSaxena is pretty preoccupied, and also it's around x-mas time so there might not be happening too much anyway. So you might have to run with the fork for now!

jimblue · 2017-12-21T10:47:09Z

Hey @aderaaij ! No worries about this, I'm already pretty happy with Lozad!
The PR will come after x-mas for sure. It's good time to stay AFK 🎉

OlehDutchenko · 2018-01-27T06:22:22Z

@aderaaij PR has conflicts. I had the same problem in my pr #80.
You need to update your fork and rebuild dist scripts for resolving conflicts.

Also, could you please consider processing of an picture element
https://github.com/ApoorvSaxena/lozad.js/blob/master/src/lozad.js#L12

OlehDutchenko · 2018-01-27T06:41:20Z

Also you need to mark the items that have already started to load.
Otherwise, duplicate method calls for image loading are possible.
As long as the source is being loaded, there is no attribute, and when the check is repeated, the code for downloading will be called again.

Pay attention to these lines of code:

Maybe you need just add new data attribute, when image is loaded.
for example:

data-loaded="true" - when data-src inserted as src
data-full-loaded="true" - when source is fully loaded

aderaaij · 2018-01-27T16:43:23Z

@dutchenkoOleg Thanks for the ideas! I'm completely aware that these don't pass the tests as I just wanted to get some opinion of the library maintainers first. As I say in my opening post:

This is a quick and dirty pull request to check if images have already loaded. This will not pass your tests yet, and doesn't yet take in account srcset either, but I wanted to get the ball rolling!

If I ever hear back about this I will definitely take your suggestions into consideration 👌

jimblue · 2018-02-07T00:03:36Z

Hi @ApoorvSaxena!

Can we have your input on this to move on and start fixing #49?

Thanks

ApoorvSaxena · 2018-02-13T11:16:26Z

@jimblue @aderaaij #86 implementation looks better, let's continue with that unless you want to add anything apart from that

aderaaij · 2018-02-13T11:38:26Z

@ApoorvSaxena Nope I'm totally good with that!

jimblue · 2018-02-13T11:47:41Z

@ApoorvSaxena yep I'm good with that too !

EDIT: after some test it seems that #86 can't replace this PR as it doesn't check if image is fully loaded...

dlecan · 2018-06-12T16:18:28Z

This PR is very interested, I need it, how can I help to make it merged?
Thank you.

danbilokha · 2021-11-24T17:30:01Z

it's shame it was never merged :c

aderaaij added 4 commits December 16, 2017 14:41

[fix] test for waiting for image

79ec94f

[add] funtion to wait for images

a064fec

[fix] a few tests for waitforimages

46f6755

[update] dist versions

c1752ac

aderaaij changed the title ~~Check if image for loaded~~ Check if image is fully loaded Dec 16, 2017

aderaaij mentioned this pull request Dec 16, 2017

Check if image is loaded? #49

Closed

aderaaij added 2 commits December 16, 2017 19:20

[add] support for fully loading background images

f082799

[remove] console.log

cc3f545

aderaaij closed this Feb 13, 2018

perosb mentioned this pull request Mar 3, 2018

🎉 Add loaded callback #86

Merged

ApoorvSaxena mentioned this pull request May 3, 2018

Changelog and Roadmap #102

Closed

ApoorvSaxena reopened this May 3, 2018

ApoorvSaxena mentioned this pull request Jul 19, 2018

On error load fallback #117

Open

danbilokha approved these changes Nov 24, 2021

View reviewed changes

Check if image is fully loaded #79

Check if image is fully loaded #79

aderaaij commented Dec 16, 2017 •

edited

jimblue commented Dec 16, 2017

aderaaij commented Dec 16, 2017

jimblue commented Dec 16, 2017 •

edited

aderaaij commented Dec 16, 2017

jimblue commented Dec 16, 2017 •

edited

jimblue commented Dec 16, 2017

aderaaij commented Dec 16, 2017

jimblue commented Dec 16, 2017

aderaaij commented Dec 16, 2017

jimblue commented Dec 17, 2017

jimblue commented Dec 21, 2017

OlehDutchenko commented Dec 21, 2017

jimblue commented Dec 21, 2017

aderaaij commented Dec 21, 2017

jimblue commented Dec 21, 2017

OlehDutchenko commented Jan 27, 2018

OlehDutchenko commented Jan 27, 2018

aderaaij commented Jan 27, 2018

jimblue commented Feb 7, 2018 •

edited

ApoorvSaxena commented Feb 13, 2018

aderaaij commented Feb 13, 2018 •

edited

jimblue commented Feb 13, 2018 •

edited

dlecan commented Jun 12, 2018

danbilokha commented Nov 24, 2021

Check if image is fully loaded #79

Are you sure you want to change the base?

Check if image is fully loaded #79

Conversation

aderaaij commented Dec 16, 2017 • edited

jimblue commented Dec 16, 2017

aderaaij commented Dec 16, 2017

jimblue commented Dec 16, 2017 • edited

aderaaij commented Dec 16, 2017

jimblue commented Dec 16, 2017 • edited

jimblue commented Dec 16, 2017

aderaaij commented Dec 16, 2017

jimblue commented Dec 16, 2017

aderaaij commented Dec 16, 2017

jimblue commented Dec 17, 2017

jimblue commented Dec 21, 2017

OlehDutchenko commented Dec 21, 2017

jimblue commented Dec 21, 2017

aderaaij commented Dec 21, 2017

jimblue commented Dec 21, 2017

OlehDutchenko commented Jan 27, 2018

OlehDutchenko commented Jan 27, 2018

aderaaij commented Jan 27, 2018

jimblue commented Feb 7, 2018 • edited

ApoorvSaxena commented Feb 13, 2018

aderaaij commented Feb 13, 2018 • edited

jimblue commented Feb 13, 2018 • edited

dlecan commented Jun 12, 2018

danbilokha commented Nov 24, 2021

aderaaij commented Dec 16, 2017 •

edited

jimblue commented Dec 16, 2017 •

edited

jimblue commented Dec 16, 2017 •

edited

jimblue commented Feb 7, 2018 •

edited

aderaaij commented Feb 13, 2018 •

edited

jimblue commented Feb 13, 2018 •

edited