This is a patch for when #891112: Replace theme_item_list()'s 'data' items with render elements gets committed into Drupal Core. I need to be able to alter the output of the facetapi links (more so than can be done with simply overriding theme_facetapi_link_inactive or theme_facetapi_link_active. So, I'm using that core patch to enable items passed to theme_item_list to be renderable arrays. The patch that will follow shortly adjusts the facetapi text link widget to make use of this (so that rather than passing in the output of theme_facetapi_link_inactive, you instead pass that instruction as a render array.

Note: I've also added indexed_value as an attribute on these link render arrays, to enable some powerful theming...

My use case:

I have a facet for a 'color' attribute in some products. I need to be able to render these links as color swatches. Usually I would be able to use hook_block_view_alter to get in and adjust the render arrays in the blocks before they get rendered. However, due to a shortcoming in Drupal core, theme_item_list does not accept render arrays on the data attribute (see above linked issue). With that Drupal core patch applied, and this patch (to follow), we now have full access to the underlaying render array from within hook_block_view_alter(). This allows me to use indexed_value to look up the relevant taxonomy term, look at the value of a field attached to that taxonomy term which has a color swatch jpeg, and then render that jpeg inplace of the standard link!

I wrote a similar patch for OG at #1332524: Use renderable arrays for links in og_node_create_links. This kind of thing allows for very powerful and flexible render array altering in the theme.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrfelton’s picture

Status: Active » Needs review
FileSize
2.24 KB
cpliakas’s picture

I understand what you are getting at. I think the problem for me is that the patch for #891112: Replace theme_item_list()'s 'data' items with render elements is slated for D8, so I am concerned about this change being backwards compatible with the current versions of D7. Also, I am confused as to what the following snippet does and why it is required:

if (isset($variables[''])) {
  $variables = $variables[''];
}

An empty key seems a little strange to me and I am trying to wrap my head around it. I would actually recommend creating a new widget that is an extension of the FacetapiWidgetLinks class. That way you can have complete control over the output matching the functionality of the patch you posted. To me it would be great if you contributed this small module back which would have the added benefit of being able to innovate more quickly than Facet API can.

By creating a widget plugin that extends FacetapiWidgetLinks, it looks like you would only have to override the FacetapiWidgetLinks:: setThemeHooks() and FacetapiWidgetLinks:: buildListItems() methods to change the theme functions and modify the render arrays respectively.

Let me know what you think of this approach,
Chris

mrfelton’s picture

The patch is now targeted at D8, although I think there is still a chance of getting it into D7. The bit of code you referenced is indeed a strange one, and really shouldn't be in any final patch. For some reason, in some rare cases I found that the render array would be returned as an array with just one item keyed on ''. Obviously for this to be committed, that would have to be resolved, and the core patch would have to be committed. I mainly posted here so I had a patch to reference in our drush make files, and didn't have time to debug the empty array issue any further.

We could create a custom widget, but really all we are trying to do is effectively theme those links so that they show a color swatch rather than a straight link - this really is purely theme level stuff. This patch is simply about making facetapi more flexible when it comes to theming - render arrays allow a lot more flexibility that strings.

cpliakas’s picture

Understood, and thanks for posting. I think that the theory of the widgets is for site builders who may or may not be themers be able to click select different displays, so even if the differences are minor from a technical perspective, it would still be good to create a widget so we could package the config up in a reusable block of code. That way non-technical site builders could take advantage of this very interesting display.

Also, if possible it would be great to get a link or screenshot of what you are doing. Sounds really cool.

cpliakas’s picture

Status: Needs review » Needs work

Marking as needs work based on comments in #3 and #4,

mrfelton’s picture

Updated patch to apply to latest dev code. It still has the problems noted above, so still marked as needs work. FYI, attached is a screenshot that shows essentially what we are doing. They are taxonomy facets. Each of the taxonomy terms has an imagefield, which has the color swatch graphics attached. In the theming, we look up the relevant taxonomy term by its indexed value, and render the image as part of the facet link.

cpliakas’s picture

What if we added a wrapper function, such as theme_facetapi_item_list(), that would allow developers to more easily override the list generation function and use something that allows for renderable arrays?

Jon Pugh’s picture

Not a bad idea, until theme_item_list() is fixed...

Attached is a patch that does that, but I'm not sure its the best way, since it replaces all 'item_list' #theme properties with 'facetapi_item_list'

A "better" way might be to hook_theme_registry_alter() and swap out our own callback for the entry for 'item_list'.

Thoughts?

ALSO: I added 'facet' as a property right next to 'indexed_value' so we can also use that to check what to alter.

Jon Pugh’s picture

Wow, I messed up that patch big time... Please ignore it.

Re-rolling against RC4 (which I'm pretty sure is dev...)

having a hard time getting it to work, at the moment...

Jon Pugh’s picture

Status: Needs work » Needs review
FileSize
6.35 KB

Ok, here is a patch, against RC1...

I don't have time at the moment to fix this against rc4... I tried, but something went wrong and I couldn't track it down.

This feature is working for this patch to RC1... it will need to be re-rolled against dev, but I can't do that today.

Jon Pugh’s picture

Version: 7.x-1.x-dev » 7.x-1.0-rc1
cpliakas’s picture

Component: Code » Contrib
Status: Needs review » Needs work

Hi careernerd.

Thanks for posting the code. At this stage of the game I don't want to make this type of change to the core module. However, Facet API is flexible enough so that you can define a new widget outside of Facet API and build the list via the facetapi_item_list() theme function as added in the patch. This could either be a standalone module or integrated into one of the Facet API Extra / Bonus projects. Let me know if you need some clarity or guidance on this direction.

Thanks for contributing,
Chris

mrfelton’s picture

I decided to make my use case into to a module rather than continually patching facetapi to add the support I needed:

The Search API swatches module provides a new display widget that extends FacetapiWidgetLinks, which allows you to select a file or imagefield attached to the relevant vocabulary in order to render it's content as the facet link.

cpliakas’s picture

That's excellent! Definitely the best approach, and I am excited to try it out.

Thanks for the great contribution,
Chris

koddr’s picture

Try implements facetapi_item_list() in actual version of module.
This code don't have results:

function MYTHEME_facetapi_item_list($variables) {
  $items = $variables['items'];
  $title = $variables['title'];
  $type = $variables['type'];
  $attributes = $variables['attributes'];

  // Only output the list container and title, if there are any list items.
  // Check to see whether the block title exists before adding a header.
  // Empty headers are not semantic and present accessibility challenges.
  $output = '<div class="item-list">';
  if (isset($title) && $title !== '') {
    $output .= '<h3>' . $title . '</h3>';
  }

  if (!empty($items)) {
    $output .= "<div" . drupal_attributes($attributes) . '>';
    $num_items = count($items);
    $i = 0;
    foreach ($items as $item) {
      $attributes = array();
      $children = array();
      $data = '';
      $i++;
      if (is_array($item)) {
        foreach ($item as $key => $value) {
          if ($key == 'data') {
            $data = $value;
          }
          elseif ($key == 'children') {
            $children = $value;
          }
          else {
            $attributes[$key] = $value;
          }
        }
      }
      else {
        $data = $item;
      }
      if (count($children) > 0) {
        // Render nested list.
        $data .= theme_item_list(array('items' => $children, 'title' => NULL, 'type' => $type, 'attributes' => $attributes));
      }
      if ($i == 1) {
        $attributes['class'][] = 'first';
      }
      if ($i == $num_items) {
        $attributes['class'][] = 'last';
      }
      $output .= '<span' . drupal_attributes($attributes) . '>' . $data . "</span>\n";
    }
    $output .= "</div>";
  }
  $output .= '</div>';
  return $output;
}

I want to change ul > li list to div > span.

Any ideas?

steinmb’s picture

Version: 7.x-1.0-rc1 » 7.x-2.x-dev

Sort of dated, and looking at the code is render arrays used, though it might still be parts that could be cleaned up. Have not looked that deeply into this issue. Still valid?

steinmb’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev

7.x-1.x is prob. the right branch