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

Using with Image lazyload or non-image tag is not rendered properly #69

Closed
weilinzung opened this issue Jan 14, 2021 · 10 comments · Fixed by #71
Closed

Using with Image lazyload or non-image tag is not rendered properly #69

weilinzung opened this issue Jan 14, 2021 · 10 comments · Fixed by #71

Comments

@weilinzung
Copy link
Contributor

weilinzung commented Jan 14, 2021

@wynfred The layout is not rendered properly when using image lazyload or has no image tags.

Example:
https://stackblitz.com/edit/ngx-masonry-layout-issue-1zeuss?file=src%2Fapp%2Fapp.component.html

<img loading="lazy" class="brick" [src]="post.image">
<div class="brick" style="background-color: red; width: 500px; height: 500px; position: relative">test test test testtest test</div>
@weilinzung weilinzung changed the title Using without Image and layout is not rendered properly Using with Image lazyload is not rendered properly Jan 17, 2021
@weilinzung weilinzung changed the title Using with Image lazyload is not rendered properly Using with Image lazyload or non-image tag is not rendered properly Jan 17, 2021
@wynfred
Copy link
Owner

wynfred commented Jan 18, 2021

Hi, thank you for submitting a PR. I'm not sure how the image loading mechanism is causing problem here. It should be pretty simple to layout if you don't have image tags. I see you have a div with fixed width and height. You need to set them for the container so the masonry can work properly. You could also check out the element sizing section in the doc https://masonry.desandro.com/options.html#itemselector

Let me know if I misunderstood your question or if you have other questions. Thanks.

@weilinzung
Copy link
Contributor Author

weilinzung commented Jan 18, 2021

@wynfred
I tried already like the example on the issue, it won’t work.

https://github.com/wynfred/ngx-masonry/blob/master/src/lib/ngx-masonry.directive.ts#L42 look at here, it is the whole part of ngAfterViewInit caused the issue. My pr could give an option to use image tag or any image lazyload methods.

have you try this yet? https://stackblitz.com/edit/ngx-masonry-layout-issue-1zeuss?file=src%2Fapp%2Fapp.component.html

  public masonryOptions: NgxMasonryOptions = {
    gutter: 10,
    itemSelector: "#item",
    columnWidth: "#item"
  };

image lazyload:

<section class="masonry">
	<ngx-masonry [options]="masonryOptions" [ordered]="true">
		<div id="item" ngxMasonryItem *ngFor="let post of posts" stlye="width: 500px; height: 500px;">

			<img loading="lazy" class="brick" [src]="post.image">

			<div class="info">
				<span class="user">@{{post.user}}</span>
				<span class="title">{{post.title}}</span>
			</div>
		</div>
	</ngx-masonry>
</section>

Non-image Tag:

<section class="masonry">
	<ngx-masonry [options]="masonryOptions" [ordered]="true">
		<div id="item" ngxMasonryItem *ngFor="let post of posts" stlye="width: 500px; height: 500px;">


			<div class="brick" style="background-color: red; width: 500px; height: 500px; position: relative">test test test testtest test</div>


			<div class="info">
				<span class="user">@{{post.user}}</span>
				<span class="title">{{post.title}}</span>
			</div>
		</div>
	</ngx-masonry>
</section>

@wynfred
Copy link
Owner

wynfred commented Jan 21, 2021

I found the problem here is the "ordered" mode was designed to work with items with image, so the non-image tags are not working. I'll think about how to handle non-image tags in "ordered" mode.
When I tried the demo with lazy loaded images, I didn't notice any issue. Could you be specific about what the issue is?

@weilinzung
Copy link
Contributor Author

weilinzung commented Jan 21, 2021

Are you testing on Safari? If you test on chrome, you should be seeing empty with Chrome native loading="lazy".

I also just added ng-lazyload-image, so image lazyload would work for both chrome and safari.

<img class="brick" [lazyLoad]="post.image">

https://stackblitz.com/edit/ngx-masonry-layout-issue-1zeuss?file=src%2Fapp%2Fapp.component.html


Basically below codes would detect if the image loaded completely or error then add to public add(newItem: NgxMasonryDirective) . If with image lazy loading, the image would never load until it is on the viewport.
Is this making sense?

     this.renderer.listen(imageRef, 'load', _ => {
          this.imageLoaded(imageRef);
        });

  this.renderer.listen(selectedElementRef, 'error', (_) => {
            tthis.imageLoaded(selectedElementRef);
          });

If you have any feedbacks, please add to this PR. It should be ok for non-image tags even with ordered or any image lazyload methods: https://github.com/wynfred/ngx-masonry/pull/71/files

Chrome:
chrome

Safari:
Screen Shot 2021-01-21 at 6 59 25 PM

@wynfred
Copy link
Owner

wynfred commented Jan 22, 2021

Hi, thanks for the info. Now I understand this problem but I'm not sure how you expect this library to work with lazy loaded images? Basically the browser would need images to be at their exact position so it can calculate the viewport distance and then load the images. However, masonry needs the images to be downloaded first so it can calculate the positions.
Your PR may work but it won't give you any benefits of lazy loading, because everything will be added at the same position at the beginning. If you need lazy loaded images, I would suggest implementing it in a different way.

@weilinzung
Copy link
Contributor Author

weilinzung commented Jan 22, 2021

Hi, thanks for the info. Now I understand this problem but I'm not sure how you expect this library to work with lazy loaded images? Basically the browser would need images to be at their exact position so it can calculate the viewport distance and then load the images. However, masonry needs the images to be downloaded first so it can calculate the positions.
Your PR may work but it won't give you any benefits of lazy loading, because everything will be added at the same position at the beginning. If you need lazy loaded images, I would suggest implementing it in a different way.

Please look at the updated readme as well: https://github.com/wynfred/ngx-masonry/pull/71/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R133

This lib basically is not supporting image lazyload. If the user needs to have lazyload, they have to skip this part this.renderer.listen. There are no other ways that users can implement image lazyload if that renderer is guarding everything.

The basic solution to fix the overlapping issue is pre-adding at least the height for each item. Isn't what you suggest as well for non-image tag?

Also, the core lib masonry-layout doesn't add imageLoaded by default, I am not sure why this one has the this.renderer.listen as default.


If detectImageLoad is true(default), so it would as normal behavior like what you say. If as false, basically just turn off the renderer. This way we can have ordered=true working as well with non-image tags layout. Here is the working demo with this new option: https://github.com/weilinzung/ngx-masonry-demo

@wynfred
Copy link
Owner

wynfred commented Jan 25, 2021

Thanks for the explanation. If you're using lazy loaded images and non-image elements, items should always be ordered. You don't need the ordered option.

To fix the lazy load problem, I would prefer not having a separate option. The library should be able to handle that by default.

And if you prefer a library that wraps the core lib and doesn't have these modifications, you can try version 1.x

@weilinzung
Copy link
Contributor Author

@wynfred thanks. And you don't have any other solutions?

I am pretty sure it is a very important piece right now to make a site faster.

@TheGlockZentix
Copy link

TheGlockZentix commented Jan 26, 2021

I have a list of different items some with images and some without.
As Wynfred said the [ordered]="true" attribute is not working with ngxMasonryItem's that doesn't contain any images.

My dirty hack until we get a fix for it is: Whenever i have a post that doesn't contain any image I'l just append this:
<img *ngIf="!item.mediaUrl" [src]="fakeImg" style="height: 0px; width:0px;opacity:0; position: absolute; top:-1000px; left:-1000px;" />

The fakeImg property is just a hardcoded 1x1px base64 type image (data:image/jpeg;base64,/9j/4QAYRXhpZgAASUkqAAgA.....)

@weilinzung
Copy link
Contributor Author

weilinzung commented Jan 27, 2021

@wynfred From the last conversation on the PR, I updated to with a new masonryLazy attribute for images. Please have a look. #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants