Improved isVisible function for images within scoll containers. #241 #258
Conversation
| setImageAndSourcesToDefault(element, defaultImagePath, useSrcset); | ||
| if (element.className && element.className.includes('ng-lazyloaded')) { | ||
| element.className = element.className.replace('ng-lazyloaded', ''); | ||
| } | ||
|
|
||
| return (scrollObservable: Observable<Event>) => { | ||
| return scrollObservable | ||
| .filter(() => isVisible(element, offset)) | ||
| .filter(() => isVisible(element, offset, window, scrollContainer)) |
There was a problem hiding this comment.
I chose to pass the window object here, which might not be the prettiest thing to do. But I didn't want to break the argument order.
Would you rather have me flip the arguments, so that _window = window and I change all of the tests to comply with the change?
There was a problem hiding this comment.
This is fine. It feels however a bit weird to have an optional argument (threshold) before a mandatory (_window) but I think we should keep threshold = 0 so it defaults to 0 if threshold is undefined, so keep it as it is.
src/lazyload-image.ts
Outdated
| } | ||
| } else { | ||
| return elementBounds.intersectsWith(windowBounds); | ||
| } |
There was a problem hiding this comment.
What's your preferred code convention for these kind of else statements? Do you prefer it like it is, or like this:
if (scrollContainer) {
const scrollContainerBounds = Rect.fromElement(scrollContainer);
if (scrollContainerBounds.intersectsWith(windowBounds)) {
const intersection = scrollContainerBounds.getIntersectionWith(windowBounds);
return elementBounds.intersectsWith(intersection);
}
return false;
}
return elementBounds.intersectsWith(windowBounds);EDIT: Sorry, not the best line selection for a comment.
|
|
||
| interface LazyLoadImageDirectiveProps { | ||
| lazyImage: string; | ||
| defaultImage: string; | ||
| errorImage: string; | ||
| scrollTarget: Object; | ||
| scrollTarget: HTMLElement; |
There was a problem hiding this comment.
Since we add a type here we may also add it in scroll-listener.ts. You do not have to do anything here, I created a separated issue: #270
| setImageAndSourcesToDefault(element, defaultImagePath, useSrcset); | ||
| if (element.className && element.className.includes('ng-lazyloaded')) { | ||
| element.className = element.className.replace('ng-lazyloaded', ''); | ||
| } | ||
|
|
||
| return (scrollObservable: Observable<Event>) => { | ||
| return scrollObservable | ||
| .filter(() => isVisible(element, offset)) | ||
| .filter(() => isVisible(element, offset, window, scrollContainer)) |
There was a problem hiding this comment.
This is fine. It feels however a bit weird to have an optional argument (threshold) before a mandatory (_window) but I think we should keep threshold = 0 so it defaults to 0 if threshold is undefined, so keep it as it is.
| is(result, false); | ||
| }); | ||
|
|
||
| it('Should not be visible when image is vertically in window\'s view, but not in scroll-container\'s', () => { |
There was a problem hiding this comment.
What do you think of using string template instead of '? Eg.
it(`Should not be visible when image is vertically in window's view, but not in scroll-container's`, () =>
Hey, here's the improved image visibility detection within a scroll container.
I'll comment some lines where I need your opinion.