kswedberg / jquery-carousel-lite

A jQuery carousel plugin based on jCarouselLite by Ganeshji Marwaha
MIT License
177 stars 59 forks source link

A few suggestions/fixes #1

Closed halilim closed 13 years ago

halilim commented 14 years ago

Instead of adding another fork to the bazillion of forks we already have, I tought that making suggestions here would be good, hence this is the best fork imho (and inludes my few modifications).

  1. Selecting only the first ul (L34):

    var div = $(this), ul = div.find('ul'), ...

    ->

    var div = $(this), ul = div.find('ul:eq(0)'), ...

    Or something like this. Why? Imagine a situation where there are ul's inside main li's themselves. If select all the ul's, then those inner li's inside the inner ul's will be affected too by the CSS modifications.

  2. New options for disabling the automatic setting of width or/and height of the slide li's. Sometimes we need to carousel HTML content instead of images. While jCarouselLite supports this, we have a few quirks here: The HTML contents each may have different heights, and setting the height of all automatically have some issues. Instead I propose two options:

    noAutoWidth: false,
    noAutoHeight: false  

    And at L53:

     li.css({width: li.width(), height: li.height()});

    ->

     if (!o.noAutoWidth) {
         li.css({width: li.width()});
     }
     if (!o.noAutoHeight) {
         li.css({height: li.height()});
     }

    Of course, there may be much better ways in order to accomplish this, but I merely intend to show that we need a proper way to include HTML's of any height.

In fact, I'm suggesting these as results of real needs, and currently using the code this way.

kswedberg commented 14 years ago

thanks so much for these suggestions! Both seem reasonable, and I'll make the changes to my repo. For the second suggestion, I'd rather have the options be autoWidth / autoHeight and have their defaults be true. Negating a negative can get confusing.

thanks again. Expect to see an update soon.

halilim commented 14 years ago

Sure for the naming, they got a bit too negative, for they arose from a need of negation :) Another naming could be setDivWidth / setDivHeight or setContainerWidth / setContainerHeight.