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...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Steven Jones’s picture

Here'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:

// Load javascript on the page in init to help fool caching.

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.

redndahead’s picture

This 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?

Steven Jones’s picture

Ah, 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.

Steven Jones’s picture

Assigned: Unassigned » Steven Jones
Steven Jones’s picture

Here'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.

redndahead’s picture

I 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?

Steven Jones’s picture

hook_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.

redndahead’s picture

Priority: Major » Critical
redndahead’s picture

Status: Needs review » Fixed

I 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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

guillaumev’s picture

Status: Closed (fixed) » Active

I'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...

guillaumev’s picture

Status: Active » Needs review
FileSize
2.18 KB

Here 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...

guillaumev’s picture

Here is a patch that applies on 3.0 (last patch applies on dev version).

wangqizhong’s picture

Status: Needs review » Fixed

Thanks 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!

NaX’s picture

Status: Fixed » Needs review
FileSize
1.81 KB

I 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.

  • redndahead committed 0e94b89 on 8.x-1.x
    - #1240642 by Steven Jones: Fixed Views slideshow js is added in...
  • wangqizhong committed e4e377b on 8.x-1.x
    Issue #1240642 by Steven Jones, guillaumev: Fixed Views slideshow js is...

  • redndahead committed 0e94b89 on 8.x-3.x
    - #1240642 by Steven Jones: Fixed Views slideshow js is added in...
  • wangqizhong committed e4e377b on 8.x-3.x
    Issue #1240642 by Steven Jones, guillaumev: Fixed Views slideshow js is...

  • redndahead committed 0e94b89 on 8.x-4.x
    - #1240642 by Steven Jones: Fixed Views slideshow js is added in...
  • wangqizhong committed e4e377b on 8.x-4.x
    Issue #1240642 by Steven Jones, guillaumev: Fixed Views slideshow js is...
NickDickinsonWilde’s picture

Issue summary: View changes
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.