Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
At the moment, views slideshow's JS and CSS is being added on hook_init:
/**
* Implements hook_init().
*/
function views_slideshow_init() {
// Load javascript on the page in init to help fool caching.
drupal_add_js(drupal_get_path('module', 'views_slideshow') . '/js/views_slideshow.js');
$vs_path = drupal_get_path('module', 'views_slideshow');
drupal_add_css($vs_path . '/views_slideshow.css');
}
With our setup, including the jcycle plugin this then accounts for 70KB of javascript being loaded on every single page, not to mention that this code is invoked on pages that have nothing to do with rendering a views slideshow.
I think that we could do with moving the calls to drupal_add_js
closer to where they actually get used, so that we only add the JS if we need to.
Patch is coming...
Comment | File | Size | Author |
---|---|---|---|
#15 | views_slideshow-hook_library-1240642-cycle-1.patch | 1.81 KB | NaX |
#13 | 1240642-hook_init-13_3.0.patch | 2.18 KB | guillaumev |
#12 | 1240642-hook_init-12.patch | 2.18 KB | guillaumev |
#5 | views_slideshow-hook_library-1240642-5.patch | 6.02 KB | Steven Jones |
#1 | views_slideshow-hook_library-1240642.patch | 5.19 KB | Steven Jones |
Comments
Comment #1
Steven Jones CreditAttribution: Steven Jones commentedHere's a patch (and a drush make friendly one) that move the adding of the javascript and CSS to a hook_library and calls to
drupal_add_library
.I'm not sure what the comment:
Is all about? Is it so that the javascript always gets aggregated? If so, nasty! Let core do it's best, and others can always change it from there.
Comment #2
redndahead CreditAttribution: redndahead commentedThis was done because of caching. Other modules are doing the same thing. It wouldn't load the javascript when cached. Now that said I had patched views to fix some bad caching a while back. Can you check to see with all caching turned on the javascript still loads?
Comment #3
Steven Jones CreditAttribution: Steven Jones commentedAh, I suspected that might have been the issue.
I'll test when I get a chance, and let you know. Views should try and attempt to record the 'out-of-band' data like CSS and JS added when caching though. Views 3 might be relying on render arrays in this case though, and the
#attached
jazz.Comment #4
Steven Jones CreditAttribution: Steven Jones commentedComment #5
Steven Jones CreditAttribution: Steven Jones commentedHere's a patch that's basically the patch from #1 re-rolled for 9a91a5f6459c324568b0146531b8303d02c4b651.
I've also added checking for libraries module, so we silently fail to add the libraries if it's not installed, as per the current code.
I tested against the latest verison released of views with and without caching, and it all worked just fine. So there doesn't seem to be an issue with caching.
Comment #6
redndahead CreditAttribution: redndahead commentedI don't think the views_slideshow_cycle.js should be added to libraries. That's really meant only for external libraries. drupal_add_js would be fine for that. Any specific reason for using libraries module?
Comment #7
Steven Jones CreditAttribution: Steven Jones commentedhook_library
is a core thing, not a libraries module thing, see: http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...Basically it's a means of packaging up any JS and CSS you need into a single place. If you ever need to add additional CSS or JS files, or extra dependencies you only need to change your hook_library implementation, rather than hunting down all the places in the code where you added the JS and CSS.
Comment #8
redndahead CreditAttribution: redndahead commentedComment #9
redndahead CreditAttribution: redndahead commentedI committed this without using hook_library. I'm not sure it's the right reason to use this. It seems a jquery_cycle.module would be the one to use this.
Thanks for testing.
Comment #11
guillaumev CreditAttribution: guillaumev commentedI'd like to reopen this because I looked at the latest dev version of views_slideshow and the drupal_add_js is still in hook_init...
Comment #12
guillaumev CreditAttribution: guillaumev commentedHere is a new patch proposing the use of hook_library again... This saves about 20 kB (non-compressed) on every page that does not need views slideshow. For my use case, it helps a LOT...
Comment #13
guillaumev CreditAttribution: guillaumev commentedHere is a patch that applies on 3.0 (last patch applies on dev version).
Comment #14
wangqizhong CreditAttribution: wangqizhong commentedThanks a lot to Steven Jones, guillaumev and everyone else for the great work on the patches, testing, reviews and reporting.
It's more time this got committed and I went ahead and had this rolled at:
7.x-3.x: e4e377b.
Marking this as fixed.
Please let me know if you would have any other questions, comments, issues or concerns on any of these changes, I would be glad to provide more information.
Thanks again to everyone for the help and great work on this issue.
Cheers!
Comment #15
NaX CreditAttribution: NaX commentedI think this change also impacts the cycle module. With the last dev version I cant have more than one working slide show.
At first I thought it had something to do with #1066096: Support for multiple slideshows on one page, but after a lot of digging I found this issue and got it working gain.
Attached is a patch that shows how I got it working. I actually think my patch my overlap both issues, but I dont know enough about the two issues to make that judgment.
If this effects contrib modules then this should probably be documented.
Hope it helps.
Comment #19
NickDickinsonWilde