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

option to bypass classList mutation? #59

Closed
leeoniya opened this issue Jul 9, 2019 · 6 comments
Closed

option to bypass classList mutation? #59

leeoniya opened this issue Jul 9, 2019 · 6 comments

Comments

@leeoniya
Copy link
Collaborator

leeoniya commented Jul 9, 2019

hey @malchata

it would be useful if yall offered an option to avoid classList mutation. it would require a bit more internal book-keeping, likely via a couple Set objects, e.g. bgLoaded and srcLoaded.

the main motivation for this is when used with a virtual-dom lib. let's say hyperscript creates an element h("img", {class: "lazy", "data-src": "123.jpg"}) but then some action causes a redraw that results in h("img", {class: "lazy active", "data-src": "123.jpg"}). the vdom lib does the diff against the prior vnode and simply resets the className when it detects the change. if yall is relying on lazyBackgroundLoaded being present or lazyClass being absent, this would break these assumptions.

@malchata
Copy link
Owner

malchata commented Jul 9, 2019

If we can maintain current functionality, I'd be good with this change. My only reservation is that I'm totally not an expert on vdom stuff, so I'm not sure how the implementation for this would work. Do you have some thoughts on how we might achieve this? I'd be particularly interested in seeing a concrete example of how this might work with React/Preact.

@leeoniya
Copy link
Collaborator Author

leeoniya commented Jul 9, 2019

I'm not sure how the implementation for this would work.

there's nothing specific yall needs to do besides avoiding classList/className mutation (and by extension, relying on classes for anything except initial presence of lazy to hook its observer magic).

If we can maintain current functionality, I'd be good with this change.

i'm pretty sure we can. i would probably opt to make this the default behavior unless you feel there's enough utility in keeping the lazyBackgroundLoaded class (if this pans out there won't be a need for it in terms of book-keeping), but since you expose it in options, maybe you felt it was useful for other purposes.

Do you have some thoughts on how we might achieve this?

pretty sure i can implement this without too much headache.

@malchata
Copy link
Owner

malchata commented Jul 9, 2019

pretty sure i can implement this without too much headache.

Let's do it! As part of a PoC, I think it'd be good to show a demo of how this might work with React or Preact.

@leeoniya
Copy link
Collaborator Author

leeoniya commented Jul 11, 2019

it turns out that due to the way background images are loaded (exclusively relying on classList mutation), this is not going to be possible without making breaking changes (and requiring something like data-bg to be specified on the element, rather than in the css.)

i went a little overboard and wrote a refactored lib for myself based on yall that has quite a lot of breaking changes: https://github.com/leeoniya/notyet.

feel free to assimilate anything you find useful.

@malchata
Copy link
Owner

malchata commented Jul 11, 2019

Ah, bummer. To be honest though, I'd consider making yall.js a new minor version that would incorporate these changes. If the breaking changes are very significant, we could just do a major version bump, write some functionality for this lib to co-exist with native lazy loading, and then possibly consider development on this thing "complete". I'm getting to a point where I'd like to see this project stabilized, especially if native lazy loading sees broader support.

What do you think?

EDIT: I made you a collaborator on the repo. Way past time anyway, considering how helpful you've been on this project.

@leeoniya
Copy link
Collaborator Author

write some functionality for this lib to co-exist with native lazy loading

https://css-tricks.com/native-lazy-loading/

hmm, maybe it makes sense to switch from querying .lazy to [loading=lazy]. and disable the lib if native lazy loading is supported (but how to check for it cheaply?). this would also alleviate class mutation concerns. of course the native impl only mentions iframes and images (not pictures). i'm extremely against adding more code to deal with interop of some far-into-the-future feature that's only half-useful in 50% of browsers.

What do you think?

it really depends on what you think of NotYet. the reason i didn't simply open 1000 PRs to yall.js is because i didn't want to discuss whether each PR was worth doing, if you liked it, etc; there are enough code formatting changes, tooling changes, api changes and docs changes that i would not be willing to throw away even 30% of my choices for the sake of backwards compat or to be co-maintainer of yall.js. my general demeanor is pretty self-serving in that if i can write/modify a tool myself, then i don't need to put up with an ounce of API awkwardness or features i'll never need.

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

No branches or pull requests

2 participants