-
Notifications
You must be signed in to change notification settings - Fork 142
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
Added support for lazyloading background images defined in stylesheets #444
base: main
Are you sure you want to change the base?
Changes from all 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 |
---|---|---|
|
@@ -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]' | ||
|
@@ -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)) { | ||
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. 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. 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. 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. 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. EDIT: See my comment below instead. |
||
return never(); | ||
} | ||
return this.hooks.getObservable(attributes).pipe(lazyLoadImage(this.hooks, attributes)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
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 results in a compilation error (it is weird that CI doesn't report this). 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. Interesting. We can definitely change it to |
||
let img: HTMLImageElement; | ||
if (isImageElement(element) && isChildOfPicture(element)) { | ||
const parentClone = element.parentNode!.cloneNode(true) as HTMLPictureElement; | ||
|
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.
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 ;)