Add zebra striping to theme_item_list()

This isn't testing for the right condition.

if ($i & 1) {
...

How about doing what theme_table does by setting up a flip array and letting it toggle back and forth. Nice and efficient.

Ah! I was thinking "&&". Your using a bitwise operator so it does work. I just never seen that done in core and that's where I learned php so, my mistake. :)

This being new to me, I'll let someone else judge on the coding style.

I prefer the ($i & 1) technique: It obviously isn’t familiar to everyone, but it is all there in one place, whereas the flip technique looks very odd until you realise that; yes, it is supposed to be using the value from the previous iteration; yes, it is initialised properly; oh, that’s what the weird array that relies on string matching is for.

$count % 2 could possibly be optimised if it didn’t mess about with negative values, so I think ($i & 1) is the most efficient.

I’ve just issued a patch for theme_item_list(), so if that gets in, and before this one, I just wanted to flag before i forget, that the code can be (line: 1459);-

    if ($i & 1) {
      $attributes['class'] = empty($attributes['class']) ? 'odd' : ($attributes['class'] .' odd');
    }
    else {
      $attributes['class'] = empty($attributes['class']) ? 'even' : ($attributes['class'] .' even');
    }
    if ($i == 0) {
      $attributes['class'] .= ' first';
    }
    if ($i == $num_items - 1) {
      $attributes['class'] .= ' last';
    } 

My IDE wont let me make a patch of this change until one is committed :-(

I am someone who to this day still divides two by shifting one to right, so believe, masking by one to test for even/odd is familiar to me. Still the class is more a PHP construct and we are coding in PHP, not C or assembly.

I know this still has an &, but is this a bit clearer? & is part of php — being part of C doesn’t mean we can’t use it if it is the most appropriate construct.

(Hope this applies against HEAD: I’ve got several uncommitted patches on theme.inc — it might need an offset)

Cool idea, patch didn't seem to apply to head anymore, rerolled patch.

Thanks for the re-roll.

You don’t say what work is needed (apart from the re-roll (which you’ve done))

Have you reviewed it? ;-)

Ow sorry, somehow slipped that select box heh :)
Did a quick review, added following php code in a page with php filter:

$options = array('one (should have first)', 'two', 'three', 'four (should have last)');
$result = theme('item_list', $options);
echo $result;

and got this

<ul>
<li class="even first">one (should have first)</li>
<li class="odd">two</li>
<li class="even">three</li>
<li class="odd last">four (should have last)</li>
</ul>

which is good :) Setting to rtbc!

Ok, I was a bit to fast settting it to RTBC. When looking at other listings in drupal core (admin/content/node or watchdog), the first row always starts with 'odd'. Maybe we should follow this logic also here.

Ok, switch odd and even, output is now

<ul>
<li class="odd first">one (should have first)</li>
<li class="even">two</li>
<li class="odd">three</li>
<li class="even last">four (should have last)</li>
</ul>

Patch looks good. It is not entirely clear how commonly used this would be though. I guess there might be a rare use case for this.

One more thought. We might be able to clean-up the pager.inc code a bit now? I believe it is adding first-page CSS classes which might now be redundant? Let's try to answer this question before we proceed. Thanks!

Hmm, I fail to see right now how this is related to pager. Pager uses it's own theme functions and pagers aren't in a nested list at this point.
(there is some debate on another issue about pager though at #294449, maybe take it to there ?)

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

Re-rolled, but using $i % 2 instead of $i & 1. On my computer there's negligible performance difference between the two (0.01 microseconds, which is a tiny fraction of the time spent in theme_item_list()), not enough to justify the inconsistency with other Drupal zebra-striping code. If someone wants to open a separate issue to change *all* Drupal code that zebra stripes to use the slightly faster $i & 1, please do so and post benchmarks.

Re #19: I don't think pager.inc is an issue any more. It uses classes that all start with 'pager-'.

You might be right that it will get bumped to D8, but let's not give up quite yet. This issue got no attention for the past year when assigned to the "theme system" component, but might get more love from people monitoring the "markup" component. I don't think it's out of scope for D7 since it's not an API change and shouldn't break backwards compatibility (unless we're concerned about breaking overly generic CSS selectors like .odd), it resolves a consistency issue (why are field items within theme_field() zebra-striped, but items within theme_item_list() not), and helps our general strategic goal of making Drupal increasingly attractive to designers and themers.

Moving to D8.

theme_table does odd/even nicely:

    $flip = array(
      'even' => 'odd',
      'odd' => 'even',
    );

and then

$class = $flip[$class];

Thanks for taking the time to report this issue.

However, marking as duplicate of #256827: Various bugs in theme_item_list(). You can follow up on that issue to track its status instead.