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.

Sign up for GitHub

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

Jump to bottom

Added support for lazyloading background images defined in stylesheets #444

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added support for lazyloading background images defined in stylesheets #444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion example/src/app/pages/bg-image.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { Component, ChangeDetectionStrategy } from '@angular/core';
min-height: 1127px;
background-position: center;
background-size: cover;
background-image: url('https://images.unsplash.com/photo-1496396297824-fdda3c54ad14?dpr=1&auto=format&fit=crop&w=1500&h=999&q=80&cs=tinysrgb');
Copy link
Owner

Choose a reason for hiding this comment

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

I'm actually surprised that this works. In previous versions of Angular the template was placed into the dom before the directive was executed so that the browser starts to download background-image before the directive could change it. But I guess this is nice ;)

}

div.ng-lazyloaded {
Expand Down Expand Up @@ -38,6 +39,6 @@ export class BgImageComponent {
images = [
'https://images.unsplash.com/photo-1470071459604-3b5ec3a7fe05?dpr=1&auto=format&fit=crop&w=1500&h=894&q=80&cs=tinysrgb',
'https://images.unsplash.com/photo-1474871256005-cbf50b902421?dpr=1&auto=format&fit=crop&w=1500&h=1000&q=80&cs=tinysrgb',
'https://images.unsplash.com/photo-1496396297824-fdda3c54ad14?dpr=1&auto=format&fit=crop&w=1500&h=999&q=80&cs=tinysrgb'
''
];
}
4 changes: 2 additions & 2 deletions src/lazyload-image.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { switchMap, tap } from 'rxjs/operators';
import { createHooks } from './hooks-factory';
import { lazyLoadImage } from './lazyload-image';
import { Attributes, HookSet, ModuleOptions, StateChange } from './types';
import { getNavigator } from './util';
import { getNavigator, isImageElement } from './util';

@Directive({
selector: '[lazyLoad]'
Expand Down Expand Up @@ -69,7 +69,7 @@ export class LazyLoadImageDirective implements OnChanges, AfterContentInit, OnDe
tap(attributes => attributes.onStateChange.emit({ reason: 'setup' })),
tap(attributes => this.hooks.setup(attributes)),
switchMap(attributes => {
if (!attributes.imagePath) {
if (!attributes.imagePath && isImageElement(attributes.element)) {
Copy link
Owner

Choose a reason for hiding this comment

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

There is an issue when setting the image path asynchronous and I think it is a bit confusing that we should use the path from css when the string is empty.

Copy link
Author

Choose a reason for hiding this comment

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

So is the issue you're bringing up here that the default image wouldn't be displayed correctly in this case with the async image? Because other than that, I can't see why it would be a problem to simply remove the inline background definition if it's an empty string...

The trick here is not to look at this like we're 'using the path from css' so much as look at it like we're 'removing the inline background definition', whether someone has defined a background in CSS is their own choice.

Copy link
Owner

@tjoskar tjoskar Mar 13, 2020 •
edited

Choose a reason for hiding this comment

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

EDIT: See my comment below instead.

return never();
}
return this.hooks.getObservable(attributes).pipe(lazyLoadImage(this.hooks, attributes));
Expand Down
3 changes: 3 additions & 0 deletions src/shared-preset/preset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ const end: FinallyFn = ({ element }) => {
};

export const loadImage: LoadImageFn = ({ element, useSrcset, imagePath, decode }) => {
if (!imagePath) {
return Promise.resolve(null);
}
Comment on lines +22 to +24
Copy link
Owner

Choose a reason for hiding this comment

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

This results in a compilation error (it is weird that CI doesn't report this). loadImage should always return a string. Like return Promise.resolve(''); or similar.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting. We can definitely change it to '' but it compiled fine for me when working with it. I think that the first thing we need to establish is whether this is the right direction to go here, and if it is then we can change this to an empty string.

let img: HTMLImageElement;
if (isImageElement(element) && isChildOfPicture(element)) {
const parentClone = element.parentNode!.cloneNode(true) as HTMLPictureElement;
Expand Down
8 changes: 6 additions & 2 deletions src/util/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ export function setImage(element: HTMLImageElement | HTMLDivElement, imagePath:
element.src = imagePath;
}
} else {
element.style.backgroundImage = `url('${imagePath}')`;
if (imagePath) {
element.style.backgroundImage = `url('${imagePath}')`;
} else {
element.style.backgroundImage = '';
}
}
return element;
}
Expand Down Expand Up @@ -49,7 +53,7 @@ function setImageAndSources(setSourcesFn: (image: HTMLImageElement) => void) {
if (isImageElement(element) && isChildOfPicture(element)) {
setSourcesFn(element);
}
if (imagePath) {
if (imagePath || !isImageElement(element)) {
setImage(element, imagePath, useSrcset);
}
};
Expand Down