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

Include jQuery above page__footer to allow for upgrade-safe custom JavaScript #1238

Closed
ohadschn opened this issue Sep 10, 2017 · 9 comments
Closed

Comments

@ohadschn
Copy link
Contributor

ohadschn commented Sep 10, 2017

Suppose I want to add some JS code that runs before _main.cs (in my case, I want to add some classes to elements to be processed by _main.js, specifically the image-popup class: #1235).

So I add a JS file to assets/js and reference it from footer/custom.html, but then I don't have jQuery because it's bundled inside main.min.js which is only included after (lower in the HTML).

I realize I could override the scripts but that would interfere with Gem upgrades.

@ohadschn
Copy link
Contributor Author

Also, getting jquery (and any other external dependency really) from a CDN rather than minifying it inside main.min.js would probably increase performance (and reduce hosting costs), both due to the CDN hosting and browser download parallelization.

@mmistakes
Copy link
Owner

Just override _includes/scripts.html

There's barely anything in that file and is very likely not to change making it safe.

As far as pulling out jQuery into a CDN version. Not going to get into that debate. There's to many Stackoverflow and blog posts arguing the benefits of doing it and not.

@ohadschn
Copy link
Contributor Author

ohadschn commented Sep 10, 2017

Override _includes/scripts.html and do what? I can already have my script included before main.min.js using footer/custom.html as I mentioned above. The problem is that I don't have jQuery. I suppose I could include it again (before my script) but that seems rather wasteful...

Regarding CDN, I took a quick look in SO in it doesn't seem like much of a debate... the overwhelming majority of answers and upvotes are clearly in favor of a CDN. It is especially recommended since HTTP/2 due to caching improvements.

@mmistakes
Copy link
Owner

Add your scripts after main.min.js which bundles jQuery with a few lines of JavaScript to init various plugins (e.g. lightbox).

I'm screwed either way I do it. Include the CDN and someone will want to self host to avoid an external dependency. Bundle it and someone will want a CDN ref'd one. I've done it both ways in the past and it's a losing battle.

I think you'll see this sort of things with themes in general. They can't possibly do ever suggested web performance suggestion and balance features/customization at the same time.

Users that are savvy enough to request this sort of stuff are likely out growing an "open sourced" theme and would be better served forking/building their own to fit needs.

@ohadschn
Copy link
Contributor Author

ohadschn commented Sep 10, 2017

I could add my script after main.min.js and add the magnificPopup() setup but I don't like it as much due to the lightbox setup duplication (~25 lines), especially if it ever changes in future versions. How about a compromise - could you expose some sort of event (simplest would probably be a jQuery custom event before the _main.js code executes?

Regarding CDN, I totally agree with your sentiment but I disagree with your premise. I would wager that if asked, the vast majority of your users would prefer the clear performance and cost improvements over the extremely slim chance that the Google CDN goes down but their site doesn't.

So I think the equation should be flipped - only extremely paranoid or savvy users with very specific scenarios (like firewalls that don't have CDNs allowed) would fork the theme. To most laymen, this is an easy, no-brainer win (the SO votes seem to strongly support this of thought)...

@mmistakes
Copy link
Owner

mmistakes commented Sep 10, 2017

Vast majority of users host for free with GitHub Pages, which already has some CDN power behind it. I'd be willing to wager performance isn't a concern or even considered in most cases. They're just happy to have a site up for free somewhere.

@ohadschn
Copy link
Contributor Author

That article suggests it only has that CDN power if configured correctly (in the case of custom sub-domains). It also doesn't address parallelization and optimized caching. And by the same token performance is not considered, I would posit that dependency on something like the Google CDN isn't either...

So given two presumably equally irrelevant options, why not take the one with the improved experience?

@ohadschn
Copy link
Contributor Author

BTW GitHub pages is probably a much bigger dependency than Google CDN...

@mmistakes
Copy link
Owner

See #1241 for feature add.

kkunapuli pushed a commit to kkunapuli/kkunapuli.github.io that referenced this issue May 30, 2019
…#1241)

* Allow `<head>` and footer scripts to be changed via config
* Update JavaScript documentation

Close mmistakes#1238
sumeetmondal pushed a commit to sumeetmondal/sumeetmondal.github.io that referenced this issue Sep 10, 2019
…#1241)

* Allow `<head>` and footer scripts to be changed via config
* Update JavaScript documentation

Close mmistakes#1238
makaroniame added a commit to makaroniame/makaroniame-old.github.io that referenced this issue May 18, 2022
…#1241)

* Allow `<head>` and footer scripts to be changed via config
* Update JavaScript documentation

Close mmistakes#1238
jchwenger pushed a commit to jchwenger/jchwenger.github.io that referenced this issue May 5, 2023
…#1241)

* Allow `<head>` and footer scripts to be changed via config
* Update JavaScript documentation

Close mmistakes#1238
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