luyadev / luya

LUYA is a scalable web framework and content management system with the goal to please developers, clients and users alike.
https://luya.io
MIT License
811 stars 205 forks source link

LazyLoad extraClass changed place #2041

Closed JohnnyMcWeed closed 4 years ago

JohnnyMcWeed commented 4 years ago

What steps will reproduce the problem?

Use latest version and LazyLoad

What is the expected result?

extraClass is still added to the image, as in the class description

What do you get instead?

The output for the LazyLoad-Widget changed: extraClass seems to be added to the wrapper instead.

Further

'attributesOnly' => true, didn't seem to work as well... Should I open another issue?

Additional infos

Q A
LUYA Version 1.6.0
nadar commented 4 years ago

@TheMaaarc does it have something to do with the overhaul?

TheMaaarc commented 4 years ago

extraClass has changed place, yes. Just so you can actually address the wrapper with css (and the img/placeholder through a nested selector). Because if I'd left the extraClass on the img tag, you wouldn't be able to identify the wrapper without another wrapper around it (or some crazy css selector).

@JohnnyMcWeed Can you rewrite your code to work with the new extraClass position? If not, do you have an example of what doesn't work for you anymore?

I'd say I'll add the extraClass position change to the changelog / upgrade files @nadar?

attributesOnly should work, so that's a bug. I will check it today and add a unit test for that option.

nadar commented 4 years ago

I'd say I'll add the extraClass position change to the changelog / upgrade files

:+1: perfect

JohnnyMcWeed commented 4 years ago

@TheMaaarc I had some images which all changed the appearance though they had different bootstrap classes on it... E. g. one of it was a square before any not it's rectangular due to the absolute position of the image and the class changes ( img-fluid rounded-circle p-3 --> was a square before, now it's rectangular and needs more stylings/classes with it around now...)

nadar commented 4 years ago

So it seems we have produced a serious BC break. Any suggestions to fix that with a patch release - without to change code?

JohnnyMcWeed commented 4 years ago

Thanks @TheMaaarc seems to be working again :) What's still open is the attributesOnly option I think. (Mentioned under "further" in this issue) At least with my BS4 theme this doesn't seem to work. The output seems to work, but it's not getting replaced. Code stays like this:

<div class="jumbotron jumbotron-fluid" data-src="/images/some-image.png" data-width="878" data-height="508" data-as-background="1">
    <div class="container">
        <h1 class="display-4">Title</h1>
        <p class="lead">This is a modified jumbotron that occupies the entire horizontal space of its parent.</p>
    </div>
</div>
JohnnyMcWeed commented 4 years ago

Interesting aswell: Seems that with e. g. bootstrap 4's fixed-top (like in a navbar) the image doesn't get replaced

TheMaaarc commented 4 years ago

@JohnnyMcWeed Hmmm - I did test the attributesOnly option, my div had a fixed height tho. Can you try to add a height of 1px to your jumbotron? I'm guessing that, because it has no height, the IntersectionObserver ignores it.

I will take a look at the fixed-top issue and create a new issue if necessary.

Thanks for reporting!

JohnnyMcWeed commented 4 years ago

Stays like this

TheMaaarc commented 4 years ago

@JohnnyMcWeed I just checked your code example again. There should be a js-lazyimage class (alongside jumbotron).
You can see it in the Test file: https://github.com/luyadev/luya/blob/master/tests/core/lazyload/LazyLoadTest.php#L37

Can you show me the source code instead of the compiled one?
Maybe you have class="jumbotron ..." alongside Svg::widget()? That would be an issue, because then you would have two "class" attributes.

JohnnyMcWeed commented 4 years ago

Can you show me the source code instead of the compiled one? Maybe you have class="jumbotron ..." alongside Svg::widget()? That would be an issue, because then you would have two "class" attributes.

Alright, must have been related to this, thanks. My fault, sorry :)

TheMaaarc commented 4 years ago

@JohnnyMcWeed Good that it works. 👍 I looked into the fixed-top issue. That happens because the IntersectionObserver only observes elements inside the documents flow, and all fixed elements are outside that flow. In most cases the fixed elements will be visible form the start anyway. And if not, maybe it would help to position them absolute and only change that to fixed once they are in the viewport.

JohnnyMcWeed commented 4 years ago

Yes indeed, just wanted to tell it as I changed it quickly and saw it :)