Looking forward to seeing some code for this

Comments

jgrubb’s picture

You bet I will. Thanks..

jgrubb’s picture

I've got v0.1 tagged in the repo. I'm curious to see how it works with another project, so if you care to test it out it's ready to be kicked around.

btopro’s picture

Component: Miscellaneous » Code
Priority: Normal » Major
Status: Active » Needs work

Issues
jquery.lazyload.js is licensed under MIT, this violates d.o. policies on code inclusion, please utilize https://drupal.org/project/libraries API instead for remote inclusion.
Then go to https://drupal.org/project/drupalorg_whitelist and request inclusion in the packaging whitelist. This will keep the legal experts happy

The next potential issue is that you are adding a .lazy class via array_push($element['content']['lazyload']['#attributes']['class'], 'lazy');
This is assuming from a screen-reader's perspective that all images are worthless and therefore potentially will not be rendered for those with a screenreader as they ignore display:none. A better pactice is to use the drupal core included element-invisible class which uses an approved clip method for hiding material. You can read more about it here -- http://zufelt.ca/blog/drupal-7-two-new-system-classes-improve-accessibility

Another critique is the use of js/css inclusion in the .info file which too throw it on all requests. Instead, include css / js via hook_page_build https://twitter.com/davereid/status/294554866649542657 to avoid inclusion at times when its not needed. You could also bootstrap it in only when the media field is included which probably would be even more optimal so we aren't assuming it's always in scope and loading js that's unneeded (also the css probably can go away based on the other recommendation).

Lastly, there are a ton of modules that provide lazyload support, what's this adding that's different enough to justify project status?

kreynen’s picture

We need to stop spreading misinformation about FOSS licensing. The 3rd party libraries on Drupal.org says that MIT is ok to include which is why JQuery Update can include multiple version of JQuery...

http://drupalcode.org/project/jquery_update.git/blob/refs/heads/7.x-2.x:...

and CKEditor for WYSIWYG can include CKEditor...

http://drupalcode.org/project/wysiwyg_ckeditor.git/blob/refs/heads/7.x-1...

MIT, LGPLv2, GPLv2 and licenses like WTFPL that allow relicensing are allowed, but only if it meets the criteria in #2..

  1. had to be modified to work with Drupal, and the modifications were not accepted by the original author.
  2. is generally difficult to find, or the specific version needed is hard to find.
  3. is no longer maintained by the original author.
  4. Any of the above exceptions needs approval by drupal.org administrators. Upfront: 90% of all exception requests are moot. Only ask for an exception if it is really required.

Of course neither JQuery nor CKEditor meet the criteria in #2 so that policy is largely ignored. The reason not to included a .js file in a module is that if every developer did that, we'd be loading the same files multiple times... or worse, loading different versions of the file.

The rest of the issues @btopro points out seem valid.

btopro’s picture

I go off of the language on https://drupal.org/project/drupalorg_whitelist which then means that https://drupal.org/node/422996 conflicts with it at some level. That's not a major issue tho so let's debate this on twitter instead of filling up this applicants queue ;)

jgrubb’s picture

First off, I have no idea how anybody even found this project. This was just a very specific piece for a project I'm building at work and I figured I'd put it in a sandbox here instead of Github. I guess I'm confused about what d.o sandboxes are. I thought it was just a little semi-private area to kick around some ideas without having to worry too much about making my code too "nice", since this isn't intended for public release.

To your points -

I'm well aware of the GPL and of sites/all/libraries. If I ever want to make this into a full fledged project, it'll conform. Sounds like it might already, though, and thanks for posting that debate here so I can stay on top of it.

I'll look into the accessibility issue, but as I understand it, since one of the things that the javascript does is set the display property on the image to "inline", screenreaders wouldn't have an issue with that, since style properties on element tags take precedence over the CSS. If JS is disabled, there's a <noscript> fallback for the image - one of the points of this module.

Regarding the .info file - this issue would really only apply if JS aggregation were off. Otherwise it gets compiled into the main site JS file like all the other JS files. The alternative is to make sure that this JS only loads on applicable pages, which would require a different aggregated JS file to be generated and served on those pages. The cost of this procedure and network call vs. serving the first aggregated JS file out of the browser cache is what makes this a debatable performance enhancement to me. I was deliberately voting for the browser cache in doing that.

Lastly, this lazyloader has a <noscript> fallback, which none of the others have. This also applies only to the media filter, and not file fields.

And like I said, I haven't applied for project status, so while I appreciate the feedback I'd more expect to get that kind of analysis if I were applying.

kreynen’s picture

Also worth looking at #1662706: Compatible with Media module

@jgrubb, I saw this because I follow https://twitter.com/drupal_modules. I tend to check out anything anyone commits related to Media and CiviCRM. The @btopro mentioned he was looking at Lazy Loader modules and I asked him to take a look at this since I hadn't had time. He approached that review as a project requesting promotion. Even if a project isn't being promoted, the policies about 3rd party code in git still apply, but you are right about sandboxes. Beyond committing code w/ a licensing conflict, large binaries, or controversial subject matter, you should be able to post a work in progress here without having to worry about supporting it.

I think the reason people open issues in sandboxes is this is functionality they want from a promoted, versioned, supported module and since you've actually started writing code, you are also likely to be interested in making that happen.