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

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

Open
teo1978 opened this issue Jul 3, 2016 · 12 comments
Open

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

teo1978 opened this issue Jul 3, 2016 · 12 comments

Comments

@teo1978
Copy link

teo1978 commented Jul 3, 2016

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
Copy link

cdp1337 commented Jul 25, 2016

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
Copy link
Author

teo1978 commented Jul 25, 2016

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
Copy link

cdp1337 commented Jul 25, 2016

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
Copy link
Author

teo1978 commented Jul 25, 2016

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
Copy link

maybe this helps: #480

@ensemblebd
Copy link

ensemblebd commented Sep 19, 2016

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
Copy link
Owner

lokesh commented Oct 26, 2016

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.

@lokesh lokesh closed this as completed Oct 26, 2016
@teo1978
Copy link
Author

teo1978 commented Oct 26, 2016

And you are closing the issue because...?

@lokesh
Copy link
Owner

lokesh commented Oct 27, 2016

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
Copy link
Author

teo1978 commented Oct 27, 2016

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

@lokesh
Copy link
Owner

lokesh commented Oct 27, 2016

@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
Copy link

mkousta commented Nov 9, 2016

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 :)

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

6 participants