Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 12 additions & 11 deletions src/lazyload-image.directive.ts
Expand Up @@ -18,13 +18,13 @@ import {
import { getScrollListener } from './scroll-listener';
import { lazyLoadImage } from './lazyload-image';

const target = typeof window !== 'undefined' ? window : undefined;
const windowTarget = typeof window !== 'undefined' ? window : undefined;

interface LazyLoadImageDirectiveProps {
lazyImage: string;
defaultImage: string;
errorImage: string;
scrollTarget: Object;
scrollTarget: HTMLElement;
Copy link
Owner

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

scrollObservable: Observable<Event>;
offset: number;
useSrcset: boolean;
Expand All @@ -34,13 +34,13 @@ interface LazyLoadImageDirectiveProps {
selector: '[lazyLoad]'
})
export class LazyLoadImageDirective implements OnChanges, AfterContentInit, OnDestroy {
@Input('lazyLoad') lazyImage; // The image to be lazy loaded
@Input() defaultImage: string; // The image to be displayed before lazyImage is loaded
@Input() errorImage: string; // The image to be displayed if lazyImage load fails
@Input() scrollTarget = target; // Change the node we should listen for scroll events on, default is window
@Input() scrollObservable; // Pass your own scroll emitter
@Input() offset: number; // The number of px a image should be loaded before it is in view port
@Input() useSrcset: boolean; // Whether srcset attribute should be used instead of src
@Input('lazyLoad') lazyImage; // The image to be lazy loaded
@Input() defaultImage: string; // The image to be displayed before lazyImage is loaded
@Input() errorImage: string; // The image to be displayed if lazyImage load fails
@Input() scrollTarget: HTMLElement; // Scroll container that contains the image and emits scoll events
@Input() scrollObservable; // Pass your own scroll emitter
@Input() offset: number; // The number of px a image should be loaded before it is in view port
@Input() useSrcset: boolean; // Whether srcset attribute should be used instead of src
@Output() onLoad: EventEmitter<boolean> = new EventEmitter(); // Callback when an image is loaded
private propertyChanges$: ReplaySubject<LazyLoadImageDirectiveProps>;
private elementRef: ElementRef;
Expand Down Expand Up @@ -81,7 +81,7 @@ export class LazyLoadImageDirective implements OnChanges, AfterContentInit, OnDe
if (this.scrollObservable) {
scrollObservable = this.scrollObservable.startWith('');
} else {
scrollObservable = getScrollListener(this.scrollTarget);
scrollObservable = getScrollListener(this.scrollTarget || windowTarget);
}
this.scrollSubscription = this.propertyChanges$
.debounceTime(10)
Expand All @@ -92,7 +92,8 @@ export class LazyLoadImageDirective implements OnChanges, AfterContentInit, OnDe
props.defaultImage,
props.errorImage,
props.offset,
props.useSrcset
props.useSrcset,
props.scrollTarget
)
))
.subscribe(success => this.onLoad.emit(success));
Expand Down
16 changes: 11 additions & 5 deletions src/lazyload-image.ts
Expand Up @@ -9,12 +9,18 @@ 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);
const intersection = scrollContainerBounds.getIntersectionWith(windowBounds);
return elementBounds.intersectsWith(intersection);
} else {
return elementBounds.intersectsWith(windowBounds);
}
}

export function isChildOfPicture(element: HTMLImageElement | HTMLDivElement): boolean {
Expand Down Expand Up @@ -110,15 +116,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))
Copy link
Collaborator Author

@sapierens sapierens Jan 1, 2018

Choose a reason for hiding this comment

The 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.
Would you rather have me flip the arguments, so that _window = window and I change all of the tests to comply with the change?

Copy link
Owner

Choose a reason for hiding this comment

The 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 (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.

.take(1)
.mergeMap(() => loadImage(element, imagePath, useSrcset))
.do(() => setImageAndSourcesToLazy(element, imagePath, useSrcset))
Expand Down
15 changes: 15 additions & 0 deletions src/rect.ts
@@ -1,4 +1,6 @@
export class Rect {
static empty: Rect = new Rect(0, 0, 0, 0);

left: number;
top: number;
right: number;
Expand Down Expand Up @@ -33,4 +35,17 @@ export class Rect {
(rect.top < this.bottom) &&
(this.top < rect.bottom);
}

getIntersectionWith(rect: Rect): Rect {
const left = Math.max(this.left, rect.left);
const top = Math.max(this.top, rect.top);
const right = Math.min(this.right, rect.right);
const bottom = Math.min(this.bottom, rect.bottom);

if (right >= left && bottom >= top) {
return new Rect(left, top, right, bottom);
} else {
return Rect.empty;
}
}
}
32 changes: 32 additions & 0 deletions test/lazyload-image.test.ts
Expand Up @@ -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', () => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`, () => 

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);
});
});

});
62 changes: 62 additions & 0 deletions test/rect.test.ts
Expand Up @@ -296,4 +296,66 @@ describe('Rect', () => {
is(result, true);
});
});

describe('getIntersectionWith', () => {
it('Should return a correctly sized Rect if two Rect\'s intersect horizontally', () => {
// Arrange
const rectA = new Rect(0, 0, 20, 20);
const rectB = new Rect(0, 10, 20, 30);

// Act
const result = rectA.getIntersectionWith(rectB);

// Assert
is(result.top, 10);
is(result.right, 20);
is(result.bottom, 20);
is(result.left, 0);
});

it('Should return a correctly sized Rect if two Rect\'s intersect vertically', () => {
// Arrange
const rectA = new Rect(0, 0, 20, 20);
const rectB = new Rect(10, 0, 30, 20);

// Act
const result = rectA.getIntersectionWith(rectB);

// Assert
is(result.top, 0);
is(result.right, 20);
is(result.bottom, 20);
is(result.left, 10);
});

it('Should return a correctly sized Rect if two Rect\'s intersect corners', () => {
// Arrange
const rectA = new Rect(0, 0, 20, 20);
const rectB = new Rect(10, 10, 30, 30);

// Act
const result = rectA.getIntersectionWith(rectB);

// Assert
is(result.top, 10);
is(result.right, 20);
is(result.bottom, 20);
is(result.left, 10);
});

it('Should return an empty Rect if two Rect\'s don\'t intersect', () => {
// Arrange
const rectA = new Rect(0, 0, 20, 20);
const rectB = new Rect(30, 30, 50, 50);

// Act
const result = rectA.getIntersectionWith(rectB);

// Assert
is(result.top, 0);
is(result.right, 0);
is(result.bottom, 0);
is(result.left, 0);
});
});
});