hey

a lot of modules are moving towards /sites/all/libraries for their external libraries/scripts, some also requiring Libraries API.

can the same be done for jCarousel?
the scripts would have to be downloaded separately, but they would then be sharable.

Comments

lpalgarvio’s picture

another idea: be part of Views Slideshow as a submodule.

quicksketch’s picture

Title: support libraries directory and Libraries API » Support libraries directory and Libraries API
Version: 6.x-1.x-dev » 6.x-2.x-dev

I'd be interested to know what benefit we'd get from this, obviously a lot of sites use Libraries API (about the same that use jQ), but that's probably because other modules that include the functionality they want depend on Libraries API.

In any case, I'm moving this request to 2.x where we'll start adding new features. The 2.x version hasn't yet even included the previous jQ module support.

ndwilliams3’s picture

Auto detection of skins using the libraries API would be another great feature. Just create a new folder and with the skin name and drop in a css file with the same name.

lpalgarvio’s picture

definitely a good point williams.
the current way just plain sucks.

quicksketch’s picture

definitely a good point williams.
the current way just plain sucks.

In every implementation I've ever done of jCarousel, I've never once used a skin. In my opinion the best approach is to add the jCarousel CSS you'd like to use into your theme and not use a skin at all. Especially in jCarousel, where specifying the width or height of the carousel is practically required unless you just so happen to want the default width and each item being 100px square.

In any case, I think it's highly unlikely that a wide library of skins become available for the jCarousel module. If we get any good contributed skins I'd be happier to include them directly in the module rather than requiring separate downloads. But like I said, I don't find skins to be very helpful to begin with, any good theme is going to restyle their carousels anyway.

ndwilliams3’s picture

quicksketch, I agree that you usually have to set a width and height for the carousel. The main reason I use custom skins is that it applies a common class to the carousels that allows for the reuse of styles. By using skins, I only need to set the height and width of the carousel. Yes, I know you could add a common css class to the view. But how many times when you are building a site do you forget the exact css class you are using for a style and have to search through your notes or css files to find it. The dropdown skins box makes it easy to apply!

lpalgarvio’s picture

rogical’s picture

+1 this is the trend to use libraries api

quicksketch’s picture

I'm not going to add an inconvience to users because it's a trend. I need a concrete benefit here. Bundling the jCarousel library directly allows the code within the jCarousel module to work specifically with that version. API changes are not uncommon in the jCarousel library.

Take for example this feature: #825692: Pager or navigation for the carousel, which requires updated jCarousel to 0.2.8 (which is issue #1196558: Update to Version: 0.2.8). If we used the libraries directory, it would require the user to update the jCarousel module (most likely through Drush or downloading the zip from drupal.org), but then the module wouldn't work correctly until the user went and downloaded jCarousel 0.2.8 from somewhere else. By bundling directly with jCarousel the module, the Drupal code and the library code always line up.

Likewise, it's just as possible that the jCarousel module would not work with the latest version of jCarousel the library, forcing the user to try and find a version that works correctly with the module. Anyone who has tried to use jQuery UI in Drupal 6 can probably relate to this problem.

I should also note Drupal 7 includes hook_library_info(), was designed to include libraries directly with modules, just like Drupal core provides jQuery, jQuery UI, Farbtastic, and other libraries.

lpalgarvio’s picture

- (libraries api) because other modules might use jCarousel, so code might get loaded in duplicate, adding overheard and collisions/problems
- (libraries api) because managing the libraries at a central spot is nice
- (moving to /sites/all/libraries) because a central repo of libraries is a good idea
- (moving to /sites/all/libraries) because manual updates on modules and drush updates might kill the library
- (moving to /sites/all/libraries) because the download will be smaller
- (moving to /sites/all/libraries) because you don't need to manually update or maintain the library code

Libraries API support does not need to be mandatory/required - it can be loaded optionally

if module_exists('libraries') {
do_something;
}

version problems can be solved by stating in the README and project page which jCarousel version should be downloaded and is supported.

it's not that complicated ;)

quicksketch’s picture

Status: Active » Closed (won't fix)

This isn't going to happen in this module. The convenience of having a module that works out-of-the-box is too great to be offset by largely theoretical benefits.

quicksketch’s picture

I might as well counter-point all of these:

- (libraries api) because other modules might use jCarousel, so code might get loaded in duplicate, adding overheard and collisions/problems

Other modules using jCarousel can (and do) depend on the jCarousel module.

- (libraries api) because managing the libraries at a central spot is nice
- (moving to /sites/all/libraries) because a central repo of libraries is a good idea

"Is nice" and "good idea" isn't exactly hard evidence here.

- (moving to /sites/all/libraries) because manual updates on modules and drush updates might kill the library

How would it "kill the library" if the library is included in the module.

- (moving to /sites/all/libraries) because the download will be smaller

Smaller by 2KB? And you'd make the user go to an entirely different website that is often subject to change to download the library separately? This argument doesn't stand as a benefit.

- (moving to /sites/all/libraries) because you don't need to manually update or maintain the library code

Maybe that's true, but it by bundling the library directly we ensure compatibility with the version of the library that is bundled with the module. As you can see in the patch here: #1196558: Update to Version: 0.2.8, the new version of jCarousel required (very small) changes to the jCarousel module in order to be compatible. Wasn't it better that the jCarousel module continued to work just fine for the months in-between jCarousel being updated to 0.2.8 and the jCarousel module getting its next release? Or that users didn't have to hunt down a 0.2.7 version from somewhere to make it so that the compatible version of the library was installed to make it work with jCarousel?

lpalgarvio’s picture

obviously the module and your ideals have changed a lot in the past year (September 12, 2010 - October 27, 2011) so my comments might seem silly now.

been so long that i hardly remember what we were discussing here

markpavlitski’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
Category: Feature request » Bug report
Issue summary: View changes
Status: Closed (won't fix) » Active

I'm reopening this ticket and marking as a bug, as bundling 3rd party libraries is now a code policy violation.

Official Drupal policy is here: https://drupal.org/node/422996 and more background here: #418844: 3rd Party Libs in CVS as well as non-GPLv2 Code in CVS

The jCarousel library is already in the packaging white-list, so nothing needs to be done there.

Edit: corrected policy link.

kreynen’s picture

Status: Active » Postponed
Issue tags: +licensingpolicy

Updating this to reflect the pending policy change. While the jCarousel Jquery code is licensed as MIT and whitelisted, each project (module, theme, distribution) that wants to commit this code to git will likely need an exception. I'm setting this as postponed until the DA finalizes the details of a new licensing team and issue queue (likely on 8/20).