Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Version: 6.x-1.1 » 6.x-2.x-dev

This is possible but only through custom coding currently. See the example at http://sorgalla.com/projects/jcarousel/examples/static_controls.html

It'd be nice to be able to build this into our Views integration (reusing the existing views pager), but so far this has proved to be a rather difficult task. My guess is we'd need to make our own pager-like navigation for such functionality. I'm moving this to the 2.x branch.

quicksketch’s picture

Title: Add 'navigator plugin' to jcarousel » Pager or navigation for the carousel

Clarifying title. Note that we definitely won't be using the "navigator" jQuery plugin, considering it's an entirely different library.

katiebot’s picture

I have implemented a numbered pager, you should be able to adapt this code to achieve what you're after. I just have this in a js file in my theme folder.

var x = $('.jcarousel-container li').length;
for (i=1; i < x+1; i++) {
	$('.view-product-search ul.pager').append('<li><a href="#">'+i+'</a></li>'); // replace '.view-product-search ul.pager' with wherever you want to add the pager
}

function mycarousel_initCallback(carousel) {
	$('.view-product-search ul.pager li a').bind('click', function() {  // replace '.view-product-search ul.pager' with whatever you added above
		carousel.scroll($.jcarousel.intval($(this).text()));
    	return false;
	});
};

$('.jcarousel-container').jcarousel({
	initCallback: mycarousel_initCallback
});
Jerome F’s picture

I tried to use this js file on my site. I tried to replace the ".view-product-search ul.pager" but I didn't manage to get it working. Could you please give us some more details about how to use this script (for non javascrip coders) ? Do we have to create the ul or does it create it by itself (I tried both with no result) ? Does it work with the latest version of jcarousel ?

Thank you for your attention

Jerome F’s picture

How is this supposed to work, please ? I tried again and didn't manage to work it out.

I put the following markup in a views footer :

<div class="jcarousel-control">
	<ul class="carousel-pager">
		<li><a href="#">1</a></li>
		<li><a href="#">2</a></li>
		<li><a href="#">3</a></li>
		<li><a href="#">4</a></li>
		<li><a href="#">5</a></li>
		<li><a href="#">6</a></li>
		<li><a href="#">7</a></li>
		<li><a href="#">8</a></li>
		<li><a href="#">9</a></li>
		<li><a href="#">10</a></li>
		<li><a href="#">11</a></li>
		<li><a href="#">12</a></li>
		<li><a href="#">13</a></li>
		<li><a href="#">14</a></li>
	</ul>
</div>

and

var x = $('.jcarousel-container li').length;
for (i=1; i < x+1; i++) {
$('.jcarousel-control ul.carousel-pager').append('<li><a href="#">'+i+'</a></li>'); // replace '.view-product-search ul.pager' with wherever you want to add the pager
}

function mycarousel_initCallback(carousel) {
$('.jcarousel-control ul.carousel-pager li a').bind('click', function() {  // replace '.view-product-search ul.pager' with whatever you added above
carousel.scroll($.jcarousel.intval($(this).text()));
    return false;
});
};

$('.jcarousel-container').jcarousel({
initCallback: mycarousel_initCallback
});

I also tried with no HTML markup at all in the <div class="jcarousel-control"></div>.
And with this markup like in http://sorgalla.com/projects/jcarousel/examples/static_controls.html :

<div class="jcarousel-control">
	<a href="#">1</a>
	<a href="#">2</a>
	<a href="#">3</a>
	<a href="#">4</a>
	<a href="#">5</a>
	<a href="#">6</a>
	<a href="#">7</a>
	<a href="#">8</a>
	<a href="#">9</a>
	<a href="#">10</a>
	<a href="#">11</a>
	<a href="#">12</a>
</div>

with .jcarousel-control a instead of .view-product-search ul.pager

You can find the carousel I'm working on at : http://elephant-studios.com/
(just at the moment it's offline but it will be available again soon).

Crell’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
Assigned: Unassigned » Crell

I'm going to take a crack at this, for Drupal 7 at least. (A backport will be left as an exercise to the reader.)

Crell’s picture

Status: Active » Needs review
FileSize
45.46 KB

And here we go.

The attached patch sticks a numeric pager above or below (configurable) the carousel itself, if so configured. It autodetects the pager size (a process that is kinda weird, so I've heavily documented its weirdness).

Note that in order for this to work we needed the setupCallback support from the latest version of jCarousel. Therefore this patch also includes the patch from #1196558: Update to Version: 0.2.8, which performs said upgrade.

quicksketch, I have a full git history if you want to commit the long form rather than just an all-in-one patch. I just need to figure out how to generate it. :-)

quicksketch’s picture

Status: Needs review » Needs work

Sorry this definitely needs to be separate issues. It looks like you upgraded jCarousel to 0.2.8 at the same time as this patch. There's already as separate issue for it over here: #1196558: Update to Version: 0.2.8. If it looks good and doesn't cause any problems, let's get it taken care of first.

Crell’s picture

Status: Needs work » Needs review
FileSize
4.85 KB

Here's the same thing but minus the patch from #1196558: Update to Version: 0.2.8. You'll need to apply that patch first, but it seems to work fine for me so far.

(God I love git... :-) )

quicksketch’s picture

Thanks Crell this looks really good! A few suggestions:

- Can we call this "navigation" internally and "Carousel navigation" to the end-user? The word "Pager" has a lot of connotations already within Drupal core and Views.
- What's the process for retheming page numbers that is described in this help text? Seems like it should be omitted (if there isn't a way) or more descriptive on how you would change these from being numbers.

t('Enable a clickable pager to jump straight to a given page. By default it will use page numbers but it can be rethemed.')
Crell’s picture

Status: Needs review » Needs work

I'll try to get back to this later this week. Renaming "pager" should be fine. The process for retheming, um, not sure, I'm not a themer. :-) Should I just remove that part of the sentence?

quicksketch’s picture

Sure, removing the sentence sounds good. Less is more sometimes. :)

Crell’s picture

Status: Needs work » Needs review
FileSize
4.98 KB

Round 3...

- Removes that sentence entirely.
- Renames "pager" to "navigation".
- As an added bonus, adds an "active" class on the clicked navigation item for extra styling goodness.

Note this still requires the upgrade patch mentioned previously.

quicksketch’s picture

Looks great Crell! I'm swamped with work, perhaps I'll try it out this weekend. Thanks!

dashals’s picture

Ditto to rolling this into a release. I tried patching it but couldn't get it working. In post 7 of this thread it says "numeric pager above or below (configurable) the carousel itself, if so configured" but i could find anywhere to configure/enable it?

Cheers

gleroux02’s picture

Crell, one slight issue that I am seeing with the patch is that there is no active item in the navigation until after the user uses the navigation controls.

Crell’s picture

FileSize
5.09 KB

Attached patch fixes #16.

dashals: You need to first apply the patch from the other issue, then in the settings for this style plugin ("format" in the new Views UI, I believe) there's a select box to show navigation before, after, or not at all.

dashals’s picture

Mmmmmm I did apply both patches last time and it didn't work. I just started with fresh copy of jcarousel and patched it and it works a treat.

I must have made noob error the first time.

Thanks for you help and top effort on adding the navigation.

Cheers
Chris

Jerome F’s picture

I've tested it twice on two different test environments, applying first the patch in #1196558: Update to Version: 0.2.8 and the one in #17, and though I can see the following in the carousel settings:

Enable navigation

None / After / Before
Enable a clickable navigation list to jump straight to a given page.

Whatever I select After or Before, whichever skin I choose, there's no pager for my carousel, there's not even the HTML elements needed for it, even hidden anywhere on the page. So I must be missing something.

Jerome F’s picture

Please find attached the test view I used with no pager showing up.

Crell’s picture

Jerome, can you check in Firebug if you're getting any Javascript errors?

Murz’s picture

I have apply patch from #1196558: Update to Version: 0.2.8 post #3 and after that - apply #17 patch from here.
All applies successfully on jCarousel 7.x-2.4-alpha3 (instead of whitespace warning in first patch).
And after that "Enable navigation (None / After / Before)" added in settings.
But I didn't see them in page before increasing .jcarousel-container height, I have added 20px in css and see navigator.
Thanks for the patch, will be good to see it in head!

And some small problem - when I switch pages via arrows, the .active element in navigator ul not changed.

Jerome F’s picture

I don't see any javascript error in firebug. Is the patch against alpha 3? I patched the dev version in git.

quicksketch’s picture

Status: Needs review » Needs work

And some small problem - when I switch pages via arrows, the .active element in navigator ul not changed.

Thanks Murz

James Andres’s picture

FileSize
5.51 KB

Here's a re-roll of 825692-pager-patch.patch that includes a fix to correctly set the active states of the navigation links.

James Andres’s picture

Status: Needs work » Needs review

Setting as needs review.

Jerome F’s picture

Ok it works, I applied both patches. In #1196558: Update to Version: 0.2.8 and in #25.

I have noticed a few things:
It appears to be limited to 13 items? (I tried with up to 68 items for testing) Could this limit be setup in the user settings in views?

No matter what settings you use for items to scroll by, if you click on each link of the pager one by one, for example "1" on then on "2" or on "13" on then on "12" the carousel scrolls inaccurately. (in other words if you click on "1" the carousel displays item 1, then if you click on "2" the carousel displays item 5).

The active links are also strange, if the carousel displays items 1 to 4, the active link is 3. If it displays items 5 to 8, the active link is 7 and when it displays items 9 to 12 the active link is 10, and finally when you display items 13 to 16 the active link is 13. If then you go backward, it will be 13 ; 12 ; 9 and 3.

Bevan’s picture

When there are two carousels on the same page, and the first one has "navigation" (as per the feature implemented in the patches in this issue) but the second one does not, there is an error about not being able to invoke an undefined function on the last line in this snippet of code:

  for (var key in settings.jcarousel.carousels) {
    var options = settings.jcarousel.carousels[key];

    // ....snip...

    if (options.navigation) {
      options.setupCallback = function(carousel) {
        $(carousel.list)[options.navigation](function() {

This occurs because the options variable is rewritten on every for iteration over settings.jcarousel.carousels, and the options of the last carousel in settings.jcarousel are used for all carousels inside of the options.setupCallback() function. (Despite var options being declared inside the for iterator, when JS compiles the code the variable declaration is effectively moved to the first line of the function's body—outside of the for iterator.)

I fixed this by using jQuery.each() to iterate over settings.jcarousel.carousels instead of the native javascript for iterator, as per the attached patch "for-each-scope.patch", which contains only this change.

This solution works because jQuery.each() calls an anonymous function for each iteration, which ensures each iteration has it's own scope and own instance of var options within each iteration's scope. This solution also reduces the risk of a similar bug occurring, and simplifies some other aspects of the code.

The performance impact should be negligible (but I have not tested). Memory use will probably suffer more than speed, but still negligibly except for pages with a very large number of carousels on them—but they will perform badly anyway.

I also used JSLint to find some other minor errors, such as missing semicolons (which would prevent compression from working) and fixed some minor whitespace inconsistencies. The attached patch "jslint-validation.patch" includes only these changes.

The attached patch"825692-28-pager.patch" includes all the changes discussed in this comment and the previous patch in this issue queue, from comment #25, but excludes the latest jCarousel update patch from September 12 (comment #8) on #1196558: Update to Version: 0.2.8.

To use or test this patch, git-checkout or download the latest 2.x-dev release, apply the latest patch from #1196558 first, then finally apply "825692-28-pager.patch". I believe it also applies to the 2.4-alpha3 release of jCarousel, but have not tested.

Bevan’s picture

Ignore these patches. See the next comment #30.

Bevan’s picture

This version of the patch includes the patch/feature from #1287104: Make buttons themable.

It also uses the new theme function from #1287104 (Drupal.theme('jCarouselButton')) to render the navigation links. This allows the content of each pager link to be customised by any module or theme, simply by overriding Drupal.theme.jCarouselButton() in a javascript file.

I also found more opportunities for simplification, de-duplication and enhancement of the code that this patch introduces; Even if #1287104 does not go in before this patch, this version of the patch is better than previous versions and should be updated exclude #1287104 and not be dependent upon it (and thus exclude theme-ability of the navigation links/buttons).

The attached patch "themable-buttons.patch" includes only the changes introduced to the patch since the previous patch in comment #28, including #1287104. (#1287104 is a requirement for the theme-ability of the navigation buttons.)

To use or test this patch, git-checkout or download the latest 2.x-dev release, apply the latest patch from #1196558 first, then finally apply "825692-30-pager.patch".

Bevan’s picture

FileSize
7.28 KB

This patch is identical to 825692-30-pager.patch, but does not include #1287104. It does however require that #1287104 be applied beforehand.

megan_m’s picture

Subscribing

doom_fist’s picture

Could this patch be parlayed or repurposed to use the buttons not as representative of the icons/slides, but rather a page of slides.

For instance, if it returned 12 results and showed four at a time, is it possible that three pager buttons could be used, corresponding to each set of 3 slides?

Bevan’s picture

doom_first: Is that not how it works already? I thought that was how it works, although I just realised I have never used it for that type of configuration.

quicksketch’s picture

Status: Needs review » Needs work

I finally got around to reviewing this patch today, looks like it still has some issues. I separated out all the unrelated issues as best I could:

I fixed this by using jQuery.each() to iterate over settings.jcarousel.carousels instead of the native javascript for iterator, as per the attached patch "for-each-scope.patch", which contains only this change.

This (and similar each() problems) were fixed in #1208276: Loop through callbackParents fails in IE8/7 and Safari when combined with other JS libraries.

I also used JSLint to find some other minor errors, such as missing semicolons (which would prevent compression from working) and fixed some minor whitespace inconsistencies. The attached patch "jslint-validation.patch" includes only these changes.

I split this out and fixed it in #1324236: Minor code style fixes and missing semi-colons.

Now related to the patch itself:

It also uses the new theme function from #1287104 (Drupal.theme('jCarouselButton')) to render the navigation links. This allows the content of each pager link to be customised by any module or theme, simply by overriding Drupal.theme.jCarouselButton() in a javascript file.

I don't think that the navigation links and the previous/next buttons should be the same theme function. While they're both anchor tags, it seems likely that a user would want to theme the navigation links but not the arrows, or vice-versa. Or that they would want to theme them in different ways.

Other problems:
- It looks like the class on the navigation was changed to "jcarousel-nagivation", but the CSS changes still refer to a class for "jcarousel-pager".
- In my testing, the navigation included 27 links for 27 items, even though there were 4 items per page. There should have only been 7 links. Additionally the last link was somehow off, "page 6" was the last page, when "page 7" should have included the last items.
- Plus this patch just needs a reroll now that all the other dependent/related and unrelated patches have all been committed.

quicksketch’s picture

- In my testing, the navigation included 27 links for 27 items, even though there were 4 items per page. There should have only been 7 links. Additionally the last link was somehow off, "page 6" was the last page, when "page 7" should have included the last items.

Ah, I found out that what I actually had were 20 items in the carousel and 27 page links being created. This was because a few of my carousel items actually had <li>'s inside of them. Since we were finding ALL <li>'s within the carousel, not just the immediate ones, it was picking up the nested lists and making pages for every single li it found anywhere. The actual paging behavior after correcting this bug works great. Still reviewing now.

quicksketch’s picture

More issues:

- $(carousel.list)[position](function() {: This construct of using a function as a parameter to DOM manipulation methods is only supported in jQuery 1.4 and higher, making it incompatible with Drupal 6 sites.
- $(listItems[i]).addClass('active');: This will only work on page numbers if they work out evenly. If you have a page with only one extra item, the last page will not be properly activated.
- This patch occupies the options.setupCallback and options.itemVisibleInCallback callbacks, and if another callback is specified in those spots they are entirely replaced. We should check if they exist and only add our replacement callbacks if they're not already used.
- Related to above, we should move the code for these callbacks into dedicated functions, as we did for options.initCallback.

Still working through all these issues in my testing, I'm just documenting as I go to keep track of the changes I'm making.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
8.3 KB

Okay, attached is my "best effort" patch with all the above complaints fixed as well as:

- The default and tango skins updated more completely with more CSS to accommodate the navigation.
- Description texts describing when the navigation will fail to have any effect, plus checks to ensure no effect on either AJAX-enabled carousels or carousels with "circular" wrap.
- Backwards-compatible approach with Drupal 6 (tested and confirmed working there too).
- Carousels with only 1 page do not get a navigation with only one page link.

However this patch still has a huge caveat, and that's that it does not work with circular carousels. Previously jQuery would spew hundreds of errors when using the navigation with a circular carousel, but in this version I simply suppress the navigation entirely if the wrap is set to circular. Considering it's extremely common to have a circular carousel with a navigation, I'd really like to get this working. Unfortunately jCarousel's class names and page names make this pretty difficult. We'd have to re-architect this approach with circular carousels in mind.

I'm leaving this patch as needs review. It's definitely functional but without circular carousel support I can't commit this patch.

quicksketch’s picture

Status: Needs review » Needs work

Well after reviewing several other jQuery plugins for carousels/rotators, it looks like this is a very uncommon/complicated feature to implement. The next version of jCarousel (0.3) looks to be a total rewrite, for better or worse, which will include built-in support for navigation with circular carousels as well as iPhone/mobile "swipe" support. The next version looks pretty cool, but it could be a long, long time before we see that code base become usable. For now this navigation support may be the best we can do.

quicksketch’s picture

FileSize
8.88 KB

Well enough thinking and this problem wasn't as hard as I feared. The patch makes it so that the navigation works great with either circular or normal wraps. Conceptually it's rather simple:

- Calculate the number of pages total.
- Always keep track of which page you're on.
- When clicking a different page, scroll the number of items per-page * page number difference.
- This makes it so that going forward (from page 2 to page 5 for example) will always scroll to the right, while going backward (from page 6 to page 1 for example), will always scroll left.

Circular carousels still have other problems (like not trimming off "old" items and just continually growing), but that's not for this issue to solve.

quicksketch’s picture

Status: Needs work » Needs review
quicksketch’s picture

Status: Needs review » Fixed

Applied and tested to Drupal 6 and everything works perfectly there too. I think this is good to go. Applied and committed to both 2.x branched.

Bevan’s picture

Woohoo!! I knew that this needed more testing and that there were still bugs and edge cases, but I did not think there were that many.

Thanks for driving this one home quicksketch! :)

Tim Jones Toronto’s picture

@quicksketch - nice work on this module btw - thank you!

Status: Fixed » Closed (fixed)

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