Markup around product images

+1 for the markup change.

Great idea, here is a little patch to test.

marking for review

One drawback I just thought of: this will probably break any modules/sites that use JavaScript/jQuery to do image handling, e.g. uc_option_images. (A lot of sites do something similar, but site-specific). On the whole, I think it's well worth it because it will make writing new JavaScript/jQuery much easier and cleaner, but it's definitely a change that should be emphasized in the release notes so that people will know WHY their site broke when they upgraded.

Should this get backported to 1.x?

Good catch,

I would be personally against porting to 1.X as we don't want to break the sites using all of the current code.

I would say this is safe to implement now in 2.x, there is a precedence in Ubercart for Beta code meaning you are still allowed to break things, for example all of the cart block markup that just got changed that broke all of stephs themes. :)

Looking through the uc_option_images 6.X code i don't see where adding 2

Looking through the uc_option_images 6.X code i don't see where adding 2 inside the existing untouched div would effect anything.

Is there something with jQuery where it would select only immediate children (ie. the img tags, in the original code) inside a div?

I don't know of any specific instances where this will break something. However, I have noticed that many of the "Live Sites" posted on ubercart.org use some custom jQuery to swap images for different colors or patterns or what have you. I know I've done this myself on some sites too. It depends on how you write your jQuery, but in general jQuery code is tightly coupled with the HTML, and if you change the DOM you probably have to tweak the jQuery that manipulates it. My sites are safe, because I've overridden theme_uc_product_image() to emit HTML that's easier to manipulate. Perhaps others used the same strategy.

Regardless, this change will help me because I should be able to do away with that override and live with the default HTML now. So I'm not opposed at all to the change - change away! Just mention it in the release notes!

Thanks for the patch thill_ I applied it and made some changes to allow for better theming without redoing too much to the existing code. Below is the bit of code I am using to display the images on the product nodes. I'm not exactly sure how to go about creating a patch so hopefully this will work for now.

I created a wrapping div to go around the entire image area as well as the single product image. Then wrapped the smaller images in their own span class for further control over theming. I tested it locally and in FF3, IE7, IE6, Safari and Opera and works well in all browsers.

Please review and let me know if all is good. Looking forward to seeing something like this in the next release, thanks.

function theme_uc_product_image($images, $teaser = 0, $page = 0) {
  static $rel_count = 0;

  // Get the current product image widget.
  $image_widget = uc_product_get_image_widget();
  $image_widget_func = $image_widget['callback'];

  $first = array_shift($images);

  $output = '<div class="product-image"><div class="product-image-single">';
  if ($image_widget) {
    $output .= '<a href="'. imagecache_create_url('product_full', $first['filepath']) .'" title="'. $first['data']['title'] .'"'. $image_widget_func($rel_count) .'>';
  }
  $output .= theme('imagecache', 'product', $first['filepath'], $first['data']['alt'], $first['data']['title']);
  if ($image_widget) {
    $output .= '</a></div>';
  }
  $output .= '<div class="product-image-more">';
   foreach ($images as $thumbnail) {
     if ($image_widget) {
       $output .= '<span class="image-thumb"><a href="'. imagecache_create_url('product_full', $thumbnail['filepath']) .'" title="'. $thumbnail['data']['title'] .'"'. $image_widget_func($rel_count) .'>';
     }
     $output .= theme('imagecache', 'uc_thumbnail', $thumbnail['filepath'], $thumbnail['data']['alt'], $thumbnail['data']['title']);
     if ($image_widget) {
      $output .= '</a></span>';
     }
   }
  $output .= '</div></div>';
  $rel_count++;

  return $output;
}

It's not an important thing, but I think I like thill's class names better. *shrug*

I don't think the spans for each thumbnail are necessary since you can select either the links or image with .more-product-images a, .more-product-images img. I also moved the closing tags for the divs out of the if ($image_widget) blocks.

Thanks Island Usurper, that code works great. Like you said I can style specifically the ".more-product-images a, .more-product-images img" code and have as much control as if it had a span around it (which was unnecessary). Thanks for cleaning it up a bit too.

I noticed that the links don't appear if no image widgets have been chosen for the image gallery. Those thumbnails are pretty hard to see, so I think people should have the option of seeing the full-size image, even if it's just a link to that file.

I can't think of any other improvements, so I committed the last patch.