The function defined in pagerAnchorBuilder should be a theme function. When it's set in the javascript, I've got to patch the module to change it. Previously (an earlier dev snapshot) when it wasn't being set, I could set a default in another javascript file in the theme.

CommentFileSizeAuthor
#7 theme_pager-593710-2.patch4.48 KBredndahead
#5 theme_pager-593710-1.patch4.48 KBredndahead
#4 views_slideshow-593710-4.patch1.68 KBAnonymous (not verified)
#2 views_slideshow-593710-3.patch2.5 KBAnonymous (not verified)
views_slideshow-pagerAnchorBuilder.patch2.11 KBAnonymous (not verified)

Comments

redndahead’s picture

redndahead commented

I like it. Didn't know that we could do this. Can you write a quick example on how you would theme this? Just so if someone runs into the issue they would be able to see and I might be able to document later.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new views_slideshow-593710-3.patch2.5 KB

The whole method I used was bunk, but it worked because it fell back to the default views slideshow pager. It was returning actual HTML output to the settings used to instantiate the slideshow, but the slideshow options want a function.

This patch works better, but I'm beginning to NOT like it because a theme function should be returning HTML and here the theme function returns a function.

nonetheless, here's a patch. It works. I can override my pager links now.

redndahead’s picture

redndahead commented 
+++ contrib/views_slideshow_singleframe/views_slideshow.js	2 Oct 2009 19:42:26 -0000
@@ -110,3 +95,30 @@ Drupal.behaviors.viewsSlideshowSingleFra
+      var classes = 'pager-item pager-num-' + (idx+1);
+      if (idx % 2) {
+        classes += ' odd';
+      }
+      else {
+        classes += ' even';
+      }

Any chances of throwing this bit outside the if (pager_type == 1) so we don't have to duplicate it? Possible problems?

This review is powered by Dreditor.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new views_slideshow-593710-4.patch1.68 KB

I think I figured out how to make this more sensible.

Check it out.

Something about the 'pager type' bothers me... it would be ideal if we could expose a list of theme functions that return pager links to the UI.

But this works for me for now.

redndahead’s picture

redndahead commented
StatusFileSize
new theme_pager-593710-1.patch4.48 KB

I think it's almost there. This one uses named indexes instead of 0, 1. It then cleans up a number of areas.

What do you think?

Anonymous’s picture

Anonymous (not verified) commented 
+++ contrib/views_slideshow_singleframe/views_slideshow_singleframe.theme.inc	3 Oct 2009 04:49:08 -0000
@@ -115,20 +115,9 @@ function theme_views_slideshow_singlefra
+  return '<div class="views_slideshow_singleframe_pager "' . $options['singleframe']['pager_type'] . '" id="views_slideshow_singleframe_pager_' . $id . '"></div>';

The only thing I don't understand is why the pager type is added to the class attribute. Views_slideshow is already doing a lot of "thick" HTML, but I know this is what Views does by default and some of it is what jquery cycle requires... Do you really need to add the pager type to the class?

Also this line contains a bug. It will cause this to be output:

<div class="views_slideshow_singleframe_pager "Numbered" id="views_slideshow_singleframe_pager_0"></div>

I'm on crack. Are you, too?

redndahead’s picture

redndahead commented
StatusFileSize
new theme_pager-593710-2.patch4.48 KB

I add the pager type so that we can style certain pager types differently than others in css. I may have numbers in some place, but images in another I need a way to differentiate between the two. Or eventually I may have jcarousel in one place and images in another so I need to know when I have what.

Nice catch with the quote. Adjusted patch attached

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

I think the goals of this feature request have been met. Thanks!

A sturdier long-term solution will be on my mind... The two pager types are basically theme variants already, and providing the numbered and thumbnail options in the UI means the code is a little less elegant. My pager type uses square bullets for the anchors. Adding a new pager type means patching the display plugin or choosing a pager type's theme function to override. However, I don't think Drupal.theme() could do a good job with it either, because it's very rudimentary. Hmm... think think think.

redndahead’s picture

redndahead commented
Status: Reviewed & tested by the community » Fixed

Thanks a bunch bangpound. This has been committed.

Status: Fixed » Closed (fixed)

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

endiku’s picture

endiku commented
Status: Closed (fixed) » Needs review

Should the Drupal.theme.prototype.viewsSlideshowPagerNumbered in the views_slideshow.js
patch return a span instead of a div? I'm having a stacking behavior since I don't see any float setting for those js created divs.

Otherwise fantastic work. I struggled for hours with the page theming until I came across this patch.

redndahead’s picture

redndahead commented
Status: Needs review » Postponed (maintainer needs more info)

Can you explain more why it should return a span instead of a div?

redndahead’s picture

redndahead commented
Status: Postponed (maintainer needs more info) » Fixed

Marking as fixed. Feel free to reopen if you still have the same issue.

Status: Fixed » Closed (fixed)

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