-
Notifications
You must be signed in to change notification settings - Fork 25.6k
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
Allow <head>
and footer scripts to be changed via config
#1241
Conversation
{% if site.head_scripts %} | ||
{% for script in site.head_scripts %} | ||
{% if script contains "://" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just "//" to allow for protocol-relative URLs (below too): https://stackoverflow.com/questions/4831741/can-i-change-all-my-http-links-to-just/27999789
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protocol less URLs are now considered an anti-pattern. It's preferable to use https
when available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, that's exactly what I answered on that very same SO thread :) Just thought you'd might want to support it, but I suppose there's really no need, any CDN today would have HTTPS...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. And I'm not sure if //
might filter out something it shouldn't be. I doubt anyone would have a path with that, but seems possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had the same concern, theoretically https://github.com/mmistakes//minimal-mistakes
works too. I think even https://github.com/mmistakes://minimal-mistakes
could work but since Liquid doesn't contain any startswith
method that I can tell, not much can be done anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Keeping things consistent with the rest of the "external URL" checks in the theme. You may be over thinking things a tad, simple is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overthinking is my middle name 😅
Good stuff! So a custom footer_scripts:
- /assets/js/vendor/jquery/jquery-1.12.4.min.js
- /assets/js/plugins/jquery.fitvids.js
- /assets/js/plugins/jquery.greedy-navigation.js
- /assets/js/plugins/jquery.magnific-popup.js
- /assets/js/plugins/jquery.smooth-scroll.min.js
- /assets/js/_main.js |
Looks about right. That should be everything. |
Great, so now anyone could do this to get a CDN-equivalent: footer_scripts:
- https://cdnjs.cloudflare.com/ajax/libs/jquery/1.12.4/jquery.min.js
- https://cdnjs.cloudflare.com/ajax/libs/fitvids/1.2.0/jquery.fitvids.min.js
- /assets/js/plugins/jquery.greedy-navigation.js # didn't see a CDN for this
- https://cdnjs.cloudflare.com/ajax/libs/magnific-popup.js/0.9.9/jquery.magnific-popup.min.js
- https://cdnjs.cloudflare.com/ajax/libs/jquery-smooth-scroll/1.7.2/jquery.smooth-scroll.min.js
# - insert whatever custom script here to run before main
- /assets/js/_main.js
# - insert whatever custom script here to run after main BTW, regarding the versions:
|
Some of those scripts are breaking changes if updated. Out of scope for this PR. |
Sure, was just wondering. |
Don't worry about it. I have it covered. Some of these I may drop or replace with lighter alternatives. Now that scripts can be added easily I don't need to bundle everything and can leave it up to the user to augment if they want. |
Cool! Could you throw me a hint on which of these I should be OK with upgrading in my override? |
Not sure. You'll have to test. If memory serves Magnific Popup had some CSS changes that I didn't want to mess with so I didn't bother updating. Greedy nav had something I needed to override to fix a styling inconsistency. The others I haven't looked at in a while. |
Understood. Thanks again for this. I guess I'll just follow the versions in the theme for now (simple enough to diff and update when a new release is out). |
Just a thought, maybe you could include something like |
…#1241) * Allow `<head>` and footer scripts to be changed via config * Update JavaScript documentation Close mmistakes#1238
…#1241) * Allow `<head>` and footer scripts to be changed via config * Update JavaScript documentation Close mmistakes#1238
…#1241) * Allow `<head>` and footer scripts to be changed via config * Update JavaScript documentation Close mmistakes#1238
…#1241) * Allow `<head>` and footer scripts to be changed via config * Update JavaScript documentation Close mmistakes#1238
Common feature request is an easy way to add scripts to
<head>
or in the footer without modifying any of the theme files.Currently this can be done by adding custom
script
elements to_includes/head/custom.html
or_includes/footer/custom.html
This pull request allows the user to add additional scripts to either<head>
or before the closing</body>
elements via their_config.yml
file.Simply add the full relative (or absolute) path(s) to relevant array and they will be added to the page.
Examples:
In the case of
footer_scripts
, if used, the default/assets/js/main.min.js
file won't be loaded and jQuery and other scripts used by the theme will need to be added to the array or replacements found. Unless you want the collapsible navigation, image lightboxes, smooth scrolling, and other niceties to stay broken ;-)Note: depending on the size of your site there may be a slight performance hit to build times as each of these config variables introduce
for
loops.Ref: #1238