search

TradeMe / ng-defer-load

MIT License
127 stars 37 forks source link

isVisible() -> element.offsetTop -> proposal of replacement #17

Closed peterpeterparker closed 4 years ago

peterpeterparker commented 6 years ago

Hi @VamsiVempati,

First of all, cool library, really cool library! I'm trying to lazy load my images in order to improve the accessibility of my website and I landed on your awesome work, thx a lot

I implemented your directive and it turned out that it was ignored by the lighthouse test. I digged a bit and it turned out that the images of my website (https://fluster.io) were loaded straight at the begin aka not lazy loaded because the images were recognized as being already displayed where actually the scroll was still at the top

In the directive https://github.com/TradeMe/ng-defer-load/blob/master/src/defer-load.directive.ts method isVisible (), the element y position is resolved doing

 let elementOffset = this._element.nativeElement.offsetTop;

which in my case always returned a small value, way smaller than the scrollPosition which seemed correct. After a couple of try I replaced the above code with the following one and it solved my problem

let elementOffset = this._element.nativeElement.getBoundingClientRect().top + window.scrollY;

doing so, my offset was correctly calculated.

Don't know if there is something really special in my website or if you think that it could be an improvement for the directive? If you think that isn't a bad idea, I could of course send a PR

Best regards David

vinayakpatil commented 5 years ago

var elementOffset = this._element.nativeElement.getBoundingClientRect().top + (window.scrollY || window.pageYOffset); works for me in IE

asgerjensen commented 5 years ago

I dont know... doesn't getBoundingClientRect() cause a full relayout calculation in some browsers?

vinayakpatil commented 4 years ago

Hi @VamsiVempati , can you please check https://github.com/TradeMe/ng-defer-load/pull/37 and share your thoughts?

vinayakpatil commented 4 years ago

HI @VamsiVempati just following up on this one, thanks!

VamsiVempati commented 4 years ago

Hi @peterpeterparker & @vinayakpatil

Very sorry, somehow seem to have missed this!

The changes suggested seem reasonable, however I'm unable to test this atm.

@mzoellner might be a bit closer to this repo now than I am, what are your thoughts @mzoellner ?

Thank you, Vamsi Vempati

peterpeterparker commented 4 years ago

Thx for the feedback @VamsiVempati

Actually I don't use this library anymore, therefore, when it comes to me, we can simply close this feature request.

vinayakpatil commented 4 years ago

@mzoellner did you get a chance to review this? Thanks

mzoellner commented 4 years ago

Hi @vinayakpatil, sorry for the delay here. We are not very active with reviewing PRs for this package. I am trying to fit it in during the next couple of days and get back to you.

mzoellner commented 4 years ago

I dont know... doesn't getBoundingClientRect() cause a full relayout calculation in some browsers?

According to https://gist.github.com/paulirish/5d52fb081b3570c81e3a this._element.nativeElement.offsetTop; also does, so it probably does not matter.

vinayakpatil commented 4 years ago

Hi @mzoellner, did you get a chance to check this? Thanks

mzoellner commented 4 years ago

Hi @vinayakpatil, really sorry this is taking so long. Could you please update your PR, so the merge conflicts are resolved ?

I will merge it in, once I have confirmed that the calculation is correct and we won't have any regression on our end.

mzoellner commented 4 years ago

Hi @vinayakpatil, I did the regression test on our end and it looks all good. Once you have updated the PR, I will merge it in.