airfranceklm / ng-lazy-image

Angular directive for loading responsive image when container (which is preventing reflow) is in viewport.
http://afklm.github.io/ng-lazy-image/
MIT License
351 stars 64 forks source link

_elementPosition returns position relative to positioned parent #5

Closed TimonVS closed 9 years ago

TimonVS commented 9 years ago

The _elementPostition function returns the position of the element with the afklLazyImage directive relative to the first parent that has a position of relative or absolute. See: http://api.jquery.com/position/.

A possible fix would be to let _elementPosition return the offset relative to the $container offset.

TimonVS commented 9 years ago

My fork with a fix https://github.com/TimonVS/ng-lazy-image. I will create a PR once I've implemented test cases.

SquadraCorse commented 9 years ago

Timon at the moment we are looking at our requirement to only use the lite version given with Angular. That's why our pull request was also fixed for IE8. Position for example is not in our lite version. I'm thinking about using classes since parent() is indeed too much attached to the html. We will have to think about it. Interesting to see your PR.

TimonVS commented 9 years ago
var _elementPosition = function(el) {
    var e = el || element;
    if (e.position) {
        return e.position().top;
    }
    return (el || !e.parent()[0]) ? e[0].offsetTop : e[0].offsetTop - _elementPosition(e.parent());
};

This is directly from lazy-image.js in this repo. So it does only use the jQuery implementation if the full version of jQuery is available.

My fix also has a vanilla JS fallback if the full version of jQuery isn't available.

SquadraCorse commented 9 years ago

Maybe it's my english :) At the moment we are looking at our own requirement to not be dependent on jQuery. Since we see a lot of polyfills and vanilla script in other modules just to work with the lite version.

it doesn't seem the right way to do it. So to make this clear we are looking to get rid of too much code. The setup were it looks at the direct parent() is wrong.

TimonVS commented 9 years ago

I understand, but the implementation doesn't need the full jQuery, it only needs the lite version that's built in in AngularJS. It CAN however benefit from the full jQuery version if that's available, just like how it's implemented now.

SquadraCorse commented 9 years ago

6 tweaked and merged