Move rendering of Spotlight formatter into theme function

Actually, it's just the $header variable, but it's still bad! Updating the issue summary.

Thanks for the patch!

I think having theme functions for header and footer is a little awkward. Could we instead have a 'panopoly_spotlight_wrapper' theme function? I wonder if we could just do something like:

$element['#theme'] = 'panopoly_spotlight_wrapper';

... and then it would be responsible for creating any header and footer, as well as calling render() to render the slides. This would also allow us to get of the code that puts the header before the markup on the first slide, which has always bothered me. :-)

Ideally, I'd also like to convert this from a theme function to a theme template, but we can look at that once we have the overall approach in place.

Thanks! I haven't had a chance to test it yet, but the patch is looking good. :-)

A couple minor points of review:

1. +++ b/panopoly_widgets.module
   @@ -388,12 +388,11 @@ function panopoly_widgets_entity_info_alter(&$entity_info) {
   +    'render_panopoly_spotlight' => array(
   +      'render element' => 'element',
   +    ),
   

   Let's call the theme function 'panopoly_spotlight_wrapper'. All our theme functions should be prefixed with the name of the module, and this is "wrapping" the spotlight slides which I think makes sense.

2. +++ b/panopoly_widgets.spotlight.inc
   @@ -191,45 +190,27 @@ function panopoly_widgets_field_formatter_view($entity_type, $entity, $field, $i
   +  // Assemble the spotlight wrapper
   +  $element[0] = array(
   +    '#theme' => 'render_panopoly_spotlight',
   +    '#items' => $items,
   +    '#settings' => $settings,
   +    '#formatter' => $formatter,
   +    '#duration' => $duration,
   +    '#pager_style' => $pager_style,
   +    'child' => array(),
   +  );
    
   -  // Assemble the tabs content
   +  // Assemble the spotlight items (rendered in render_panopoly_spotlight())
      foreach ($items as $delta => $item_data) {
   -    $element[$delta]['#markup'] = (($delta == 0) ? $header : '') . theme('panopoly_spotlight_view', array('items' => $item_data, 'delta' => $delta, 'settings' => $settings));
   +    $element[0]['child'][] = array(
   +      '#theme' => 'panopoly_spotlight_view',
   +      '#items' => $item_data,
   +      '#delta' => $delta,
   +      '#settings' => $settings
   +    );
   

   I'm not sure about putting this all under $element[0]. Look at the examples on the API page that you linked, when the elements can be rendered individually, its always putting the individual items under $element[$delta] and in this case, the items can be rendered individually.

   Could we do it like:

     // Assemble the spotlight wrapper
     $element = array(
       '#theme' => 'render_panopoly_spotlight',
       '#items' => $items,
       '#settings' => $settings,
       '#formatter' => $formatter,
       '#duration' => $duration,
       '#pager_style' => $pager_style,
       'slides' => array(),
     );
   
     // Assemble the spotlight items (rendered in render_panopoly_spotlight())
     foreach ($items as $delta => $item_data) {
       $element['slides'][$delta][] = array(
         '#theme' => 'panopoly_spotlight_view',
         '#items' => $item_data,
         '#delta' => $delta,
         '#settings' => $settings
       );
     }
   

   This also changes from 'child' to 'slides', so the theme function would need to be updated too, but I think 'slides' is a little easier to understand for the themer.

Thanks again for your work on this!

Thanks for the updated patch! It looks good. :-)

Changing $element[0] to $element didn't work for me.
I think it's because hook_field_formatter_view expects for it's return value: 'A renderable array for the $items, as an array of child elements keyed by numeric indexes starting from 0.'
I've put a comment in the code so people know why $element[0] was used.

Ok, thanks, your comment and explanation make sense - I'm cool with this now. :-)

One super minor nitpicky thing:

+++ b/panopoly_widgets.spotlight.inc
@@ -191,45 +190,28 @@ function panopoly_widgets_field_formatter_view($entity_type, $entity, $field, $i
+  // Assemble the spotlight wrapper
+  // $element[0] rather than $element because hook_field_formatter_view() expects a renderable array for the $items, as an array of child elements keyed by numeric indexes starting from 0.

Can you wrap comments to 80 characters? Thanks!

I still have to do some manual testing of the patch (so far I've just been doing code reviews) and I'd still really like to convert these from theme functions to templates, but we can address that in a follow-up issue as to not hold this one up.

Thanks! This worked great in my testing, so, committed. :-)

I created a follow-up issue about converting the theme functions to templates: #2532238: Convert spotlight theme functions into templates

Updated title to reflect that this went to a function, not a template.