When using jcarousel module, you should not be required to install the jcarousel library as this is now included with jcarousel project. I've attached a patch that basically checks if the jcarousel module exists and if it does uses the existing jcarousel js code. Perhaps we should make jcarousel project a dependency?

Comments

aburrows’s picture

aburrows’s picture

StatusFileSize
new2.88 KB
aburrows’s picture

StatusFileSize
new2.88 KB
jomarocas’s picture

patch # works for me

netw3rker’s picture

Status: Active » Needs work

I like the idea of combining resources with the jcarousel module team especially since the two modules do the same thing, just via a different mechanism. But, I'm not sure the two will ever quite make that happen. This is mainly because they both provide views handlers. This basically means that If you have one, you really have no reason to use the other, a team needs to standardize on one particular path.

That being said, I'm game for encouraging convergent evolution between the two modules, and this is a good step. There are a couple of issues with the patch that I'd like to see addressed first, and those guys will also need to get motivated to get their 3.x branch more accessible [see my points below after that] before I can see this go in.

1) Your patch does degrade properly which is good, but it doesn't test for version dependencies. right now that's not a problem because jcarousel-7.x-2.x is the most commonly available version, but when/if they release 7.x-3.x this code will fail because paths wont line up correctly. if you can get a version check in, that'll make me feel better.

+  if (module_exists('jcarousel')) {
+    $jcarousel_path = drupal_get_path('module', 'jcarousel');

2) the includes of the jcarousel js libraries go directly to their packaged versions (not supposed to do that - see below) I would much prefer a call to a jcarousel function that loads the libraries from "somewhere", and our code doesn't need to be conscious of it. - you might have to work with them on that.

+      if (file_exists($jcarousel_path . '/lib/jquery.jcarousel.min.js')) {
+        $jcarousel_path .= '/lib/jquery.jcarousel.min.js';
+      }
+      // Otherwise use non-minified version if available.
+      elseif (file_exists($jcarousel_path . '/lib/jquery.jcarousel.js')) {
+        $jcarousel_path .= '/lib/jquery.jcarousel.js';
+      }
+      else {
+        $jcarousel_path = FALSE;
+      }

here's my discussion point mentioned earlier:

The Drupal standard for handling external libraries is to not distribute them w/in a module - which is what the jCarousel module (incorrectly) does w/in the 7.x-2.x line. There, the version of jcarousel that its using is backdated to (I think) 0.2.4 whereas the most recent 2.x line is 0.2.9. They do have a hidden 3.x dev branch but AFAIK they don't plan to make the leap to 0.3.X anytime soon. I like the idea of coordinating resources, but they don't seem to be moving very fast on this either.