Problem/Motivation

We have two very similar templates in Drupal links.html.twig and item-list.html.twig. Both of these templates loop through a set of items, and create a HTML list. This creates twice as much code to maintain, and two separate, nearly identical, things to keep up to date.

Proposed resolution

Let's remove links.html.twig, add item-list--links.html.twig and call theme('item_list__links') to generate link lists. This implies adding the heading tag functionality to item-list.html.twig and converting the template to handle keyed item lists, and turning those keys like links.html.twig currently does

Remaining tasks

  1. #2032645: Replace calls to theme('item_list') with calls to theme('links') for links, when a heading or wrapper div is warranted
  2. #1842140: Remove title and wrapper div from item-list.html.twig
  3. #939462: Specific preprocess functions for theme hook suggestions are not invoked
  4. Replace links.html.twig with theme('item_list__links') to generate the link list items throughout core (this issue)

User interface changes

None.

API changes

TBD

Postponed by: #939462: Specific preprocess functions for theme hook suggestions are not invoked
#1813426: [meta] Consolidate all item list templates and add theme_hook_suggestions
#1804614: [meta] Consolidate theme functions and properly use theme suggestions in core
#1939064: Convert theme_links() to Twig

Original report by @RobLoach

There's a function available to create an HTML list of items (theme_item_list), and there's a function to create an HTML list of links (theme_links). Wouldn't it make sense to make the function that creates a list of links take advantage of the function that creates the HTML list of items?

This patch makes theme_links use theme_item_list to create the list of links.

Comments

RobLoach’s picture

FileSize
2.88 KB
Failed: Failed to apply patch. View

Whoops, overlapping $attributes...... Fixed.

keith.smith’s picture

(not worth changing status, but if you reroll, there's a comment in there with no ending period.)

RobLoach’s picture

Status: Needs review » Needs work

Ah, yeah... I'm constantly impressed by your good eye, Keith.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
Failed: Failed to apply patch. View
Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

Todd Nienkerk’s picture

+1. This would be very, very useful. At the very least, a conversion function would be tremendously helpful -- i.e., something that can convert the $links normally sent to theme_links() into something theme_item_list() can understand.

Todd Nienkerk’s picture

Here's my initial stab at at converting links to an item list:

function links_to_item_list($links, $title = NULL, $type = 'ul', $attributes = NULL) {
  $items = array();

  foreach ($links as $link) {
    if (isset($link['href'])) {
      // Pass in $link as $options, they share the same keys.
      $items[] = l($link['title'], $link['href'], $link);
    }
    else if (!empty($link['title'])) {
      // Some links are actually not links, but we wrap these in <span> for adding title and class attributes
      if (empty($link['html'])) {
        $link['title'] = check_plain($link['title']);
      }
      $span_attributes = '';
      if (isset($link['attributes'])) {
        $span_attributes = drupal_attributes($link['attributes']);
      }
      $items[] = '<span'. $span_attributes .'>'. $link['title'] .'</span>';
    }
  }

  return theme('item_list', $items, $title, $type, $attributes);
}
sun.core’s picture

Version: 7.x-dev » 8.x-dev
sun’s picture

Title: Make theme_links use theme_item_list » Make theme_links() use theme_item_list()

Not sure whether this still makes sense when considering HTML5's <navigation> element? (http://www.alistapart.com/articles/previewofhtml5)

Jacine’s picture

Yes plz!!! :)

sun’s picture

There's a non-obvious difference between links and item_list: links use the passed-in array keys as class names for the LIs.

Fabianx’s picture

Fabianx’s picture

Category: feature » task
Priority: Minor » Normal
sun’s picture

Copying over an essential comment from #1777326: Replace theme_links() with theme_item_list():

Hm. Closely related: #1300744: Introduce #type 'links'

I agree that it just creates a list, but there is actually quite some preprocessing involved.

My main goal of aforementioned issue was/is to retain the raw data of links in a list of link as long as possible, so modules are able to adjust the links prior to rendering (which is not easily possible currently).

I also wanted to get proper #access handling in there, so e.g., a theme would be able to output an inactive placeholder element for a link that the current user is not able to access (which is a typical feature request of designers/product/usability folks, so as to "hint" not yet registered users to what functionality they're missing by not being registered and so on -- whereas the theme/design wants to output the links/placeholder in the identical position).

However, in the end, I think what I'm aiming for is render array support for individual items in an item list. — That's a slightly weird but important difference between theme_links() and theme_item_list() right now, because only the former allows you to specify the link list items as arrays (so the link text + href + attributes are properly retained and only rendered into a string when the links are actually rendered).

tim.plunkett’s picture

jenlampton’s picture

Issue summary: View changes

updated summary

jenlampton’s picture

Issue summary: View changes

link

jenlampton’s picture

FileSize
1.46 KB
FAILED: [[SimpleTest]]: [MySQL] 56,420 pass(es), 19 fail(s), and 1 exception(s). View
jenlampton’s picture

Status: Needs work » Needs review
jenlampton’s picture

Issue summary: View changes

link

Status: Needs review » Needs work

The last submitted patch, drupal-theme_links_should_call_theme_item_list-311011-16.patch, failed testing.

drupalninja99’s picture

Well one thing, is that now we are going to need to update several automated tests to compare the rendered links with <div class="item-list">. That div is present in theme_item_list but I suppose not present in theme_links. Is that okay?

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
4.92 KB
FAILED: [[SimpleTest]]: [MySQL] 56,886 pass(es), 20 fail(s), and 1 exception(s). View

@Jen, I have updated 3 automated tests to include the wrapper div that theme_item_list adds. I bet that will fix some of these fails.

Status: Needs review » Needs work

The last submitted patch, drupal-theme_links_should_call_theme_item_list-311011-20.patch, failed testing.

johnnydarkko’s picture

Assigned: Unassigned » johnnydarkko
drupalninja99’s picture

Oh okay, well then my patch #20 doesn't make sense if the wrapper markup is being removed.

steveoliver’s picture

#23: Yes.

Besides the issue with this wrapper (which I think we can safely keep in the test until the other issue removes the wrapper), @johnnydarkko and I found out the cause of [all?] the failing tests--the missing key class on list items previously set by theme_links in theme.inc:1723:

      $class = array();
      // Use the array key as class name.
      $class[] = drupal_html_class($key);

Expecting we want to maintain this key class, the solution to the issue is to implement a preprocess function for adding the class for links lists:

function template_preprocess_item_list__links()

And of course this preprocess won't get picked up until one or both of these issues are resolved:

#939462: Specific preprocess functions for theme hook suggestions are not invoked and/or #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()

In the interim, I believe we should be able to get the preprocess function firing by explicitly adding the suggestion to drupal_common_theme():

    'item_list__links' => array(
      'variables' => array('items' => array(), 'list_type' => 'ul', 'attributes' => array()),
      'preprocess functions' => array('template_preprocess_item_list__links'),
      'base hook' => 'item_list',
    ),

So, in summary, to move forward on this, I think:

1. Leave the wrapper in tests until #1842140: Remove title and wrapper div from item-list.html.twig is fixed and/or RTBC.
2. Implement preprocess for item_list__links to add key class back to each item in 'items'.

markcarver’s picture

markcarver’s picture

In reality though, we should simply replace any calls to theme('links') or '#theme' => 'links' with simply item_list__links and just remove this completely unnecessary theme implementation. We'd still keep the suggestion prepare hook though.

markcarver’s picture

Issue summary: View changes

twig

mitokens’s picture

akalata’s picture

With #1939064: Convert theme_links() to Twig in head, theme_links() no longer exists. How is this issue affected?

jwilson3’s picture

Status: Needs work » Closed (works as designed)

We now have two templates: links.html.twig and item-list.html.twig and although they both produce very similar HTML code, the "template logic" in them is quite different so I would be inclined to either:

Option A:

Close this as works as designed.

Option B:

Eat our own dog food: remove links.html.twig and implement a real template suggestion in core: item-list--links.html.twig.

We need to account for all of the differences from links.html.twig including:

  • Add the ability to define heading level, attributes and text. Note: heading text no longer makes a lot of sense in the context of an item-list. The wrapper div and heading has already been removed from item-list.html.twig and this could be enough of an argument to go for option A above and leave these two templates as-is.
  • Add the ability to have keyed items (and turn those keys into classes).
markcarver’s picture

Title: Make theme_links() use theme_item_list() » Replace links.html.twig with item-list--links.html.twig
Status: Closed (works as designed) » Needs work
Related issues: +#939462: Specific preprocess functions for theme hook suggestions are not invoked

This really postponed on this related issue, ultimately. Now that it's in, "Option B" is actually a reality and the original intent of this issue. It still needs to happen... at some point.

However, given how close to RC we are, maybe it should be postponed for 8.1.x or 9.x? There is not really a good/current policy towards BC breakages for themes/templates/CSS in core.

jwilson3’s picture

Issue summary: View changes

I identified a couple questions/clarifications while cleaning up the issue summary. The other issue #1842140: Remove title and wrapper div from item-list.html.twig states:

Call theme('links') when you need a wrapper div and/or a title.
Call theme(item_list) when you only need a list.

However, a wrapper div and title have nothing to do with whether a list has links or not, so this could certainly be improved. Assuming that issue is not blocked/postponed and will get in before this one, then we should really figure out and discuss a strategy here that improves/clarifies DX and TX at the same time.

1. We should improve the template so wrapper/title is independent of links. For this, I can see two options (don't know which is better, but am leaning towards the latter):

  • Add the "list with wrapper div and title" functionality to the general item-list.html.twig template. By default theme('list_items') would not print the wrapper/title. Then devs could call theme('list_items__wrapped') and we specify in the preprocess to use 'div' as the wrapper tag, and 'h2' as the title tag.

OR

  • Leave list-items.html.twig as is and implement the list-items--wrapped.html.twig that adds the wrapper and title functionality and then imports (or uses twig macro) to leverage existing link-list.html.twig code for the list rendering.

2. A list of links is semantically-speaking, just "navigation". So we could use semantic <nav> wrapper and <hX> for title tag, with aria landmarks (a la pager.html.twig) and then either leverage link-list--wrapped.html.twig or implement another template link-list--navigation.html.twig that hardcodes these tags into a template that a themer could easily target and override if desired.

andypost’s picture

Version: 8.0.x-dev » 9.x-dev
Status: Needs work » Postponed

links.html.twig is a part of stable theme

catch’s picture

Version: 9.x-dev » 8.2.x-dev
Status: Postponed » Active

Moving back to 8.2.x

We can't actually remove theme hooks/templates in minor releases, but we can remove usages and deprecate them.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.