Item Lists using even/odd rows
NancyDru - November 16, 2007 - 22:02
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | NancyDru |
| Status: | patch (code needs review) |
Description
I realize it makes the code more complex, but it would be nice if we had the option of using the even and odd class separation on item lists (theme_item_list) like we can get on tables.
#1
#2
Patch based on 6-rc3.
#3
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.
#4
The flip technique is interesting; I've never seen that one before. When did theme_table add that instead of "$count%2"?
It looks to me like $i is the "row" number of the list, so it works. Note that is not just a test - it is also an AND that looks at the low-order bit of the number. I think that's faster than the "%2" technique. Certainly my limited testing came out correctly, although I was a bit surprised because I always think of the first row as being 1 (odd) instead of 0 (even).
#5
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.
#6
Yes, the first time I saw that, I thought the same thing and had to go look at the manuals to realize what it was doing. I figured that it was faster, so I just adopted the practice. I do like that "flip" technique too. I wonder which is the fastest?
#7
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 % 2could 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);-
<?php
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 :-(
#8
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.
#9
Well, as an old assembler programmer, I try to code for speed as well as readability (that's why we have comments). I haven't benchmarked it, but I have to believe
$i & 1is considerably faster than$i % 2and the flip technique. When I think about all the lists that show up on my sites, this might be enough savings to make shared hosting (by far the majority) just a tad better.#10
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)
#11
#12
Cool idea, patch didn't seem to apply to head anymore, rerolled patch.
#13
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? ;-)
#14
Ow sorry, somehow slipped that select box heh :)
Did a quick review, added following php code in a page with php filter:
<?php$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!
#15
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.
#16
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>
#17
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.
#18
I personally would use it a lot. I suspect you'll see more and more uses for it, just like in tables.
#19
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!
#20
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 ?)