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

images class attribute is lost #18

Closed
xanisu opened this issue May 16, 2018 · 6 comments
Closed

images class attribute is lost #18

xanisu opened this issue May 16, 2018 · 6 comments
Assignees
Labels

Comments

@xanisu
Copy link

xanisu commented May 16, 2018

Once yall has executed, my images are losing it's class property and a width and height attribute is forced to appear.

That fails with my needs, because I'm using that image class to use it as object-fit:'cover' in it's className.

@malchata
Copy link
Owner

malchata commented May 16, 2018

So, I wrote a comment at first and thought it was a bug with my script, but it may be your app design. The default class for lazy loading is lazy. When images are loaded, the lazy class is removed with this line in yall.js:

lazyElement.classList.remove(options.lazyClass);

I would recommend against styling the lazy class, and instead offload that styling to another class. classList.remove shouldn't drop the entire class attribute, only the specified class.

That said, can you link me to someplace where this code is running so I can see what's happening?

@xanisu
Copy link
Author

xanisu commented May 16, 2018

I've just made a simple and 'temp' fix in a fork here

'https://github.com/xanisu/yall.js/blob/addAttributes/src/yall.js'

The problem is that class attr is complete removed.

Take a look at that example at codepen.io, I've just copied yall raw code:

https://codepen.io/xanisu/pen/odaGgX

Please, note that final image should have it's style 'object-fit:contain' in order to be correctly displayed.

@malchata
Copy link
Owner

Ah, I understand the problem now!

While your fix addresses your specific situation, I think looping over the attributes present on the original element is probably best to avoid issues with dropping other necessary attributes. We also don't want to drop width and height indiscriminately because some may rely on those attributes to prevent layout shifting.

I won't be merging the temp fix, but it does help me to find a solution that will cover more use cases. I'll notify here when I have a fix that works, and you can give it a try when I have it. :)

Thanks for this!

@malchata
Copy link
Owner

malchata commented May 16, 2018

For the relevant section, I think this is closer to what you want: https://github.com/malchata/yall.js/blob/master/dist/yall-2.0.1.min.js

I need to perform more testing before I push this into a release, but if this fix works for you, feel free to use it. Just be aware that it has not been widely tested across browsers yet!

malchata added a commit that referenced this issue May 16, 2018
@xanisu
Copy link
Author

xanisu commented May 17, 2018

Works like a charm!

Thanks for sharing this mate.

@malchata
Copy link
Owner

No sweat! I'm going to keep this issue open in the meantime while I test. When I release version 2.0.1, I'll close this and tag you so you can get the official release.

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

No branches or pull requests

2 participants