New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved isVisible function for images within scoll containers. #241 #258
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,12 +9,22 @@ import { Observable } from 'rxjs/Observable'; | |
import { getScrollListener } from './scroll-listener'; | ||
import { Rect } from './rect'; | ||
|
||
export function isVisible(element: HTMLElement, threshold = 0, _window = window) { | ||
export function isVisible(element: HTMLElement, threshold = 0, _window: Window, scrollContainer?: HTMLElement) { | ||
const elementBounds = Rect.fromElement(element); | ||
const windowBounds = Rect.fromWindow(_window); | ||
elementBounds.inflate(threshold); | ||
|
||
return elementBounds.intersectsWith(windowBounds); | ||
|
||
if (scrollContainer) { | ||
const scrollContainerBounds = Rect.fromElement(scrollContainer); | ||
if (scrollContainerBounds.intersectsWith(windowBounds)) { | ||
const intersection = scrollContainerBounds.getIntersectionWith(windowBounds); | ||
return elementBounds.intersectsWith(intersection); | ||
} else { | ||
return false; | ||
} | ||
} else { | ||
return elementBounds.intersectsWith(windowBounds); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's your preferred code convention for these kind of 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. |
||
} | ||
|
||
export function isChildOfPicture(element: HTMLImageElement | HTMLDivElement): boolean { | ||
|
@@ -110,15 +120,15 @@ function setLoadedStyle(element: HTMLImageElement | HTMLDivElement) { | |
return element; | ||
} | ||
|
||
export function lazyLoadImage(element: HTMLImageElement | HTMLDivElement, imagePath: string, defaultImagePath: string, errorImgPath: string, offset: number, useSrcset: boolean = false) { | ||
export function lazyLoadImage(element: HTMLImageElement | HTMLDivElement, imagePath: string, defaultImagePath: string, errorImgPath: string, offset: number, useSrcset: boolean = false, scrollContainer?: HTMLElement) { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine. It feels however a bit weird to have an optional argument ( |
||
.take(1) | ||
.mergeMap(() => loadImage(element, imagePath, useSrcset)) | ||
.do(() => setImageAndSourcesToLazy(element, imagePath, useSrcset)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -210,6 +210,38 @@ describe('Lazy load image', () => { | |
|
||
is(result, true); | ||
}); | ||
|
||
it('Should not be visible when image is horizontally in window\'s view, but not in scroll-container\'s', () => { | ||
const element = generateElement(800, 0, 1200, 1200); | ||
const scrollContainer = generateElement(0, 0, 700, 1200); | ||
const result = isVisible(element, 0, _window, scrollContainer); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of using string template instead of '? Eg.
|
||
const element = generateElement(0, 800, 1200, 1200); | ||
const scrollContainer = generateElement(0, 0, 1200, 700); | ||
const result = isVisible(element, 0, _window, scrollContainer); | ||
|
||
is(result, false); | ||
}); | ||
|
||
it('Should not be visible when image is not in window\'s view, but is in scroll-container\'s', () => { | ||
const element = generateElement(1400, 0, 1200, 1200); | ||
const scrollContainer = generateElement(1300, 0, 1200, 1200); | ||
const result = isVisible(element, 0, _window, scrollContainer); | ||
|
||
is(result, false); | ||
}); | ||
|
||
it('Should be visible when image is in window\'s and scroll-container\'s view', () => { | ||
const element = generateElement(100, 0, 1200, 1200); | ||
const scrollContainer = generateElement(0, 0, 700, 1200); | ||
const result = isVisible(element, 0, _window, scrollContainer); | ||
|
||
is(result, true); | ||
}); | ||
}); | ||
|
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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