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.

Sign up for GitHub

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

Jump to bottom

WordPress plugin. #33

Closed
nextgenthemes opened this issue Oct 11, 2019 · 5 comments
Closed

WordPress plugin. #33

nextgenthemes opened this issue Oct 11, 2019 · 5 comments
Labels
question Further information is requested

Comments

@nextgenthemes
Copy link

Thanks @mfranzke for linking to my WP Plugin. I actaully wanted to tell you but forgot about it and then found it already in the readme.

As always I spend way more time on this then expected. The latest versions do not wrap any element that is already wrapped in noscript and also I made a change that I do not wrap the the very first element inside the the_content and put loading=eager on it. This polyfill does nothing with eager right? And wrapping it would be counterproductive as that needs the JS to execute first b4 the image would be loaded right?

If you or anyone reading this has any suggestions for the plugin please let me know.

I recently after I first released my plugin found this https://wordpress.org/plugins/native-lazyload/ by google. It got a lot of negative feedback. I skimmed the code and they do include a script but let it load with defer and make it only execute once. I like this more as you can async load it in the head, makes way more sense to me. As for the PHP code its seems quite complex, possibly bloated. Mine takes only ~150 lines so far. There are special hoops they have to jump because their script is not working on later loaded images.
@mfranzke mfranzke added the question Further information is requested label Oct 13, 2019
@mfranzke
Copy link
Owner

mfranzke commented Oct 14, 2019 •
edited
Loading

Let me begin by thanking you for providing your WP Plugin! ;-)

And I especially do like your approach of both how you handle the (first) image attribute to "eager" and even also caring about simple and non-bloated code.

  1. You're right, the polyfill doesn't do anything about "eager" images as to my impression the logic and heuristics behind it were either too complex or technically not possible to implement via JavaScript (at least previously) and on the other hand still in a learning and improving process by the Chromium folks. So after all of those have been settled (and even also some other browser vendors have contributed with their feedback and implementations further) probably some new feature regarding this attributes value might be included in an update of this polyfill.

  2. You could actually even also already load this polyfill asynchronously. And later loaded images will be included with the implementation for this issue: Integrate with client side rendering frameworks like react #19

@nextgenthemes
Copy link
Author

  1. I guess it makes no sense to implement loading="eager" in js at all because loading and executing the js is probably slower then just doing nothing. You are saying its not implemented in Chromium at all? Did not know that, I just quickly looked at some overview of the new attribute and some blog suggested making the first element eager so I did that. But not wrapping it will cause the browsers to load it normally, and that should be pretty fast considering all the other images are wrapped in noscript. I am just guessing, did not do any tests.

  2. My plugin actaully already loads your polyfill in the <head> with <script async. I am confused by

with the implementation for this issue: #19

I do not understand what you mean by that. The issue is still open it seems to be waiting for some special thing for frameworks. In your demo https://output.jsbin.com/codelib/1 its loaded with async I do not see how React and other frameworks relate to WordPress, React is used on the backend inside Gutenberg but most themes do not used any JS frameworks on the frontend so it seems fine just how it is.

@mfranzke
Copy link
Owner

I'm sorry, my comments might have been misleading on some aspects:

  • "You are saying its not implemented in Chromium at all?" -> loading="eager" is implemented in Chromium, I just wanted to mention that to my impression they are still in the process of improving it further and I'd like to wait some time until it's probably further improved regarding their learnings.
  • By mentioning the issue Integrate with client side rendering frameworks like react #19 I just wanted to point out that the functionality to rerun the polyfills functionality on any images or iframes that are being inserted into the DOM after the polyfills functionality has been initially executed will be developed in the context of that issue Integrate with client side rendering frameworks like react #19 - a functionality like this is mainly needed in case of somebody using some Frontend JS framework like react etc., which you wouldn't need for your use case, as you further explained in your last comment explained. But loading the polyfill asynchronously itself is even already possible within the current version as you've pointed out yourself.

@nextgenthemes
Copy link
Author

Ah OK thanks for the clarification.

@mfranzke
Copy link
Owner

mfranzke commented Oct 17, 2019 •
edited
Loading

@nextgenthemes, to my understanding, this issue was mainly about having this conversation and even also aligning those topics - or is there anything else that you want to do with this ticket? Elsewhere I could close it, as we seem to have aligned on everything.

mfranzke added a commit that referenced this issue Oct 17, 2019
@mfranzke
docs: mentioning loading="eager" #33
6ca05fe 
and some further improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question Further information is requested
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mfranzke @nextgenthemes