First of all, I've gotta say this is an amazing theme, but I'm encountering this issue quite a lot with the bootstrap theme: We have many overridden core functions (for this example I'll use bootstrap_pager()), and often it's necessary to add/remove/change classes and markup for the output of these functions.

In this case, we want to add a class conditionally to the pager wrapper (currently classed with "text-center" but there is no way I can think to do this without copying the entire function into our theme and modifying that one line. This causes issues with updates to the bootstrap theme when bug fixes/feature enhancements come through as we'd have to manually merge the changes into our overridden function. What would be ideal is if there was a tpl.php file for these "wrapper" type markup, that way instead of:

    return '<div class="text-center">' . theme('item_list', array(
      'items' => $items,
      'attributes' => array('class' => array('pagination')),
    )) . '</div>';

You could have something like:

    $items = theme('item_list', array(
      'items' => $items,
      'attributes' => array('class' => array('pagination')),
    ));
    return theme('bootstrap_pager', array('items' => $items));

And bootstrap_pager would simply be a template file with:

<div class="text-center">
 <?php print $items; ?>
</div>

This would give several benefits:

1) Overriding the markup would be easy, just override the template file
2) You could do preprocessing on the $items without forking the function.
3) Probably others...

However I understand that this may add extra overhead that is not wanted/desired for the theme, and if we were to do it for 1 function, then we'd want to standardise the whole theme (which may add too much overhead). I wouldn't even know where to go about doing this as I haven't wrapped my head around the whole registry.inc style of doing things but I just wanted to raise this and see what people's opinions were. We have similar issues with panels_bootstrap_layouts as well but that's a separate thing :)

Cheers

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markhalliwell’s picture

Status: Active » Postponed (maintainer needs more info)

Thanks!

I would like to avoid creating an entirely new theme hook. It's not really necessary. How about we use a theme_hook__SUGGESTION, like container__pager instead?

  $build = array(
    '#theme_wrappers' => array('container__pager'),
    '#attributes' => array(
      'class' => array(
        'text-center',
      ),
    ),
    'pager' => array(
      '#theme' => 'item_list',
      '#items' => $items,
      '#attributes' => array(
        'class' => array('pagination'),
      ),
    ),
  ));
  return drupal_render($build);

And in your sub-theme you could create a container.vars.php file and insert the following:

/**
 * Implements hook_preprocess_container__SUGGESTION().
 */
function SUBTHEME_preprocess_container__pager(&$variables) {
  $element = &$variables['element'];
  // Provide some conditional logic here.
  if (isset($variables['#some_condition_variable'])) {
    // Add custom class.
    $element['#attributes']['class'][] = 'pull-right';

    // Optionally, remove Bootstrap's centering class around the pager.
    $key = array_search('text-center', $element['#attributes']['class']);
    if (!$key !== FALSE) {
      unset($element['#attributes']['class'][$key]);
    }
  }
}
markhalliwell’s picture

Title: Returning markup in theme function overrides causes issues with overriding bootstrap defaults » Pager wrapper markup is hardcoded and isn't easily replaced
Category: Feature request » Bug report

FWIW, I think this is really a bug since it was this project introduced the wrapper in the first place. I am also updating the title to better better summarize the issue.

markhalliwell’s picture

Issue tags: +Needs change record

Also, this will need a change record in case a sub-theme has overridden Bootstrap's override.

PPS lol: Feel free to create a patch. I'd be happy to attribute credit to you for discovering this :)

acbramley’s picture

Status: Postponed (maintainer needs more info) » Needs work

Thanks very much, great to see a good solution :) I'll try to get on to a patch some time today.

Cheers

  • markcarver committed 0b33ce4 on 7.x-3.x
    Issue #2321691 by markcarver: Pager wrapper markup is hardcoded and isn'...
markhalliwell’s picture

Version: 7.x-3.x-dev » 8.x-3.x-dev
Assigned: Unassigned » neardark
Status: Needs work » Patch (to be ported)

I went ahead an committed the first code block in #1. This issue still needs a change record added though.

@neardark, I was going to apply this (or attempt to) on 8.x-3.x, but it seems we no longer have a template (or func) file for pagers? Where did this go?

neardark’s picture

Assigned: neardark » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
5.52 KB

  • neardark committed 12d5275 on 8.x-3.x
    Issue #2321691: Pager wrapper markup is hardcoded and isn't easily...
neardark’s picture

  • neardark committed 7b2d578 on
    Issue #2321691 by neardark, markcarver: Adding pager twig template
    
neardark’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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