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

LazyLoad extraClass changed place #2041

Closed
JohnnyMcWeed opened this issue Jul 25, 2020 · 13 comments · Fixed by #2042
Closed

LazyLoad extraClass changed place #2041

JohnnyMcWeed opened this issue Jul 25, 2020 · 13 comments · Fixed by #2042
Assignees
Labels

Comments

@JohnnyMcWeed
Copy link
Contributor

JohnnyMcWeed commented Jul 25, 2020

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
Copy link
Contributor

nadar commented Jul 26, 2020

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

@TheMaaarc
Copy link
Member

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
Copy link
Contributor

nadar commented Jul 27, 2020

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

👍 perfect

@JohnnyMcWeed
Copy link
Contributor Author

@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
Copy link
Contributor

nadar commented Jul 27, 2020

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

@JohnnyMcWeed
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@TheMaaarc
Copy link
Member

TheMaaarc commented Jul 29, 2020

@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
Copy link
Contributor Author

Stays like this

@TheMaaarc
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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

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

Successfully merging a pull request may close this issue.

3 participants