search

lokesh / lightbox2

THE original Lightbox script (v2).
http://lokeshdhakar.com/projects/lightbox2/
MIT License
6.24k stars 1.77k forks source link

At the bottom of the body? Are you serious?? #531

Open teo1978 opened 8 years ago

teo1978 commented 8 years ago

I've just replaced lightbox 2.7.1 with 2.8.2. To start with, I would expect it to be a 100% backward-compatible drop-in replacement that would work out of the box where the previous version used to work, otherwise you should call it 3.x.

Then I found out what the breaking change is:

Include the Javascript at the bottom of your page before the closing </body> tag:

The old version used to work by placing it in the <head>. Besides the fact that a decently designed script would work either way, having it in <head> is far more elegant and it is the way most modern frameworks and libraries work.

You are going backwards, making things worse rather than better. I'm definitely done with Lightbox.

cdp1337 commented 8 years ago

I do tend to agree with @teo1978 that it's generally good coding etiquette to bump the major version when releasing backwards-incompatible work, although I personally had absolutely no issue with upgrading to this latest version in my framework. In fact, I prefer scripts being around </body> given performance reasons.

So, -1 from me on this "bug report".

teo1978 commented 8 years ago

I personally had absolutely no issue with upgrading to this latest version in my framework

Of course, since as you say, you were already putting the script at the end of body.

I prefer scripts being around given performance reasons.

Good for you, but a lot of other people prefer scripts in the <head>.

Actually I've never seen a library before that requires you to put it in a specific place.

This is not just about backwards compatibility (which is of course a big deal in itself). Not working unless the library is placed at the end of the body is a bug, whether it affects you in particular or not.

cdp1337 commented 8 years ago

Not working unless the library is placed at the end of the body is a bug, whether it affects you in particular or not

And a toaster not keeping food cold is also a bug if that's what you're hoping for it to do. However in this case the original author's doc states

Include the Javascript at the bottom of your page before the closing </body> tag:

Seems pretty clear to me how to install the script.

teo1978 commented 8 years ago

However in this case the original author's doc states

Include the Javascript at the bottom of your page before the closing tag: Seems pretty clear to me how to install the script.

Yeah, I forgot the part where it's actually documented.

However that's just a bad design decision. There's no good reason for imposing an unnecessary and easily avoidable limitation that goes against widespread common practices (whether they are best practices or not)

And a toaster not keeping food cold is also a bug if that's what you're hoping for it to do

To make a better analogy, this would be a freezer that only keeps food cold if you put it in the south side of the kitchen. It is advertised on the box, though, so it's perfectly reasonable.

frankroch commented 8 years ago

maybe this helps: #480

ensemblebd commented 8 years ago

Why would you want to put a non-critical script in the header? it goes against SEO practices, and causes load-blocking so your page is slower. The only benefit to keeping it in the head, other than looking pretty, would be if you are using the defer and async attributes, at which point you would have to wrap it in an on-load function anyway. Which is the entire point of putting it in the body, and at the end. So that it is loaded last when the document is ready. And at the least, your page visibly loads before it starts downloading the addon resource!

I'm sorry but I had to comment, this doesn't make any sense to me. There are far more critical issues at hand. Not to be dismissive or rude, that's not my intent - i apologize in advance if it came across that way. I just don't understand why anyone would want this!! I mean I've literally written code to regex process php output in order to get hardcoded head scripts out of there into the body in wordpress due to poor design practices... I can't imagine actually wanting to put something non critical into it!

We overuse the head these days, and it really should only have minimal css and meta tags, and only ONLY super critical resources, like a raw dump of jquery (so its available instantly, and compressed into the html output). The head should be redefined as "only what is needed to load the above-the-fold page goes here", where the word "load" has complete separation from "display" when it comes to body contents.

Especially when google, bing, etc require your page to load under 1 second to get a good ranking. Async & footer loading all the way man, hands down.

lokesh commented 8 years ago

Closing out old issues.

One point for the record: Breaking changes in non-major version bumps. Some history... Lightbox started before semantic versioning became the norm. When version 2 come out, it was a complete re-write, with a brand new feature set. It was branded as a Lightbox2. A new shiny thing. I am reserving major version bumps for these large branded changes. A couple of breaking changes did slip in during v2.x. But, I do not plan to make any more breaking changes till v3.

For fun, you can still take a peek at Version 1 here.

teo1978 commented 8 years ago

And you are closing the issue because...?

lokesh commented 8 years ago

I appreciate all the discussion in this thread but I'm only keeping issues open that have actionable items as they are bug reports or feature requests.

teo1978 commented 8 years ago

What's not "actionable" about this? It should be actually pretty easy to fix.

lokesh commented 8 years ago

@teo1978 just for you, I'm fixing it. It'll be in the next patch release. :-)

The enable and build methods required the body tag to be in the DOM. Simply delayed their execution till the document is ready.

mkousta commented 8 years ago

I know this issue has been resolved but I think you don't need to delay execution until document is ready. Just a thought here, wouldn't just replacing here $('body') with $(document) resolve the issue? I think by doing this, even if someone puts the script in head tag, event delegation will do the work. Just a suggestion. You can ignore it if you have reasons to prefer waiting for document ready, if not, I could do the patch :)