search

hongaar / unveil2

:mount_fuji: A very lightweight jQuery plugin to lazy load images
http://nabble.github.io/unveil2
38 stars 11 forks source link

Multiple container and destroy #30

Closed vogdb closed 7 years ago

vogdb commented 7 years ago

Depends on #27 Functionality to have multiple unveil instances on page and destroy unveil instance.

vogdb commented 7 years ago

Don't forget to update docu! (to myself)

hongaar commented 7 years ago

Hey @vogdb thanks for your multiple PR's! Will review asap.

vogdb commented 7 years ago

Does this contain any backward incompatible changes?

Well, destroying now happens differently. We should trigger destroy.unveil instead of just taking off listeners. The rest is compatible.

Dist version goes from ~2kb to ~3kb, which is a ~50% increase. Can these changes be compressed somehow?

I've reused unveil const everywhere I could. Not sure about other parts. Logging can be definitely optimised if we move console.log call to separate function. Is 3kb that painful?

vogdb commented 7 years ago

@hongaar please look again

hongaar commented 7 years ago

@vogdb Thanks, now merged!

Will remove the call to Sauce for now until I figure out what's tripping it up so we have a proper build status again :)

Concerning the filesize, it's easy to add code and difficult to remove code, so I think it's a good to critically consider each feature which adds weight to the resulting lib. It's just one dependency of probably several for most websites, and I always like it when I can keep vendor bytes down to a minimum.

As for the console.log calls, they're automatically removed in the dist version.

vogdb commented 7 years ago

Thank you so much!