theme_field adds the odd/even classes incorrectly because it uses the modulus operator incorrectly.

$classes = 'field-item ' . ($delta % 2 ? 'odd' : 'even');

Maybe this should be switched to the way that theme_table does it (it's pretty slick).

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

and then

$class = $flip[$class];
CommentFileSizeAuthor
#32 zebra-1103340-22-reroll-32.patch4.3 KBdanquah
#22 drupal-1103340-22.patch4.26 KBtim.plunkett
#19 zebra-1103340-19.patch4.16 KBc4rl
#16 zebra-1103340-16.patch5.32 KBidflood
#15 zebra.patch5.67 KBjbrown
#11 field_odd_even-1103340-11-d7.patch771 bytesidflood
#5 field_odd_even-1103340-5.patch771 bytesidflood

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Worst case scenario, just switch it to $classes = 'field-item ' . ($delta % 2 ? 'even' : 'odd');

Jeff Burnz’s picture

Jeff Burnz commented

Done we just need to switch the classes around, this is what I'm doing in my theme_field overrides:

$classes = 'field-item ' . ($delta % 2 ? 'even' : 'odd');

edit: lol, took me so long to get around to this tim got there first, nice one man.

fubhy’s picture

fubhy commented

While swapping the classes is maybe the easier approach we should consider using the code snippet from theme_table as suggested in my initial post. Its probably also faster.

timb’s picture

timb commented

Bump, would love to see this implemented.

idflood’s picture

idflood commented
Version: 7.0 » 8.x-dev
Status: Active » Needs review
StatusFileSize
new field_odd_even-1103340-5.patch771 bytes

Here is a patch with the cool $flip thing.

jbrown’s picture

jbrown commented

Jeff's approach is much cleaner.

Is $delta guaranteed to be sequential?

fubhy’s picture

fubhy commented

I have to disagree. I find that the $flip code is much cleaner and really slick. Afaik $delta should be guaranteed to be sequential.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

It's already being used in theme_table(), and it's much easier to follow than a modular arithmetic wrapped in a ternary if statement. As evidenced by this issue, it's easy to get wrong!

idflood’s picture

idflood commented

At least we should have the same method for theme_field and theme_table. I don't see any reason why these should be handled differently.

Another idea would be to add a little helper function like get_even_odd_class($index). This would be a DRY approach but it would add a little overhead. And the benefits from having a function aren't so big, so the patch proposed in #5 is the best approach I think.

damien tournoud’s picture

damien tournoud commented
Issue tags: -theme_table +Needs backport to D7

Works for me. The sooner the better here, or themes will all implement it wrong :)

idflood’s picture

idflood commented
StatusFileSize
new field_odd_even-1103340-11-d7.patch771 bytes

Just in case, here is the d7 backport patch ( exactly identical as the one for d8 in #5 )

jbrown’s picture

jbrown commented

This should really be in a separate API function. Saves developers from re-implementing it everywhere.

Having the inline ternary statement is a performance optimization.

Having to implement this $flip array every time we need an odd/even class is not a good idea.

fubhy’s picture

fubhy commented

Do you have evidence that the modular arithmetic is faster?

damien tournoud’s picture

damien tournoud commented

This is a pattern. We don't need nor want a function here.

jbrown’s picture

jbrown commented
Title: odd/even in wrong order in theme_field » Zebra classes start with even instead of odd
Priority: Major » Minor
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new zebra.patch5.67 KB

@fuhby my comment about performance was about calling a function versus in-lining. There would wouldn't be a measurable performance difference between using modular arithmetic versus the $flip array.

One of the goals of a pattern is that it is easy to replicate. Copying and pasting a tertiary operator is easy. Implementing $flip means you have to think every time. It is also more code.

In theme_table() it makes sense to use $flip as not every row has a zebra class.

The reason the existing tertiary pattern doesn't start with 'odd' is because the counter starts at 0, not 1. Themers want the first row (0) to be odd.

My patch replaces the 7 instances of the tertiary pattern with

($count % 2) ? 'even' : 'odd'

I made a slight modification to theme_table() so it is clearer it will start with odd.

Do we document patterns anywhere?

idflood’s picture

idflood commented
StatusFileSize
new zebra-1103340-16.patch5.32 KB

#15: Make sense to use a simple and common pattern. I will keep the $flip in my mind. The patch looks good but I wasn't able to apply it with 'git apply file.patch' so I added "a/" and "b/" and reformatted a little the patch while keeping the exact same content.

I've tested this patch against the current issue related to fields and it is working. Not sure about the other changes it introduce.

jbrown’s picture

jbrown commented

#16: zebra-1103340-16.patch queued for re-testing.

c4rl’s picture

c4rl commented
Status: Reviewed & tested by the community » Needs work

This isn't quite right. In template_preprocess() and template_preprocess_block(), the counter starts with 1, not 0.

If we start counting with 0, we need

$i % 2 ? 'even':'odd';

if we start counting with 1, we need

$i % 2 ? 'odd':'even';
c4rl’s picture

c4rl commented
StatusFileSize
new zebra-1103340-19.patch4.16 KB

Patch takes #18 into account.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Not sure whether this can be backported. Potentially breaks custom themes.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

Patch looks good but needs a quick re-roll for 8.x. Mark RTBC after the re-roll.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new drupal-1103340-22.patch4.26 KB

#999040-18: Coding standards: Alternative control structure syntax in theme templates was all that prevented it from applying cleanly.

fubhy’s picture

fubhy commented 
+++ b/core/modules/block/block-admin-display-form.tpl.phpundefined
@@ -44,7 +44,7 @@
+      <tr class="draggable <?php print ($row % 2) ? 'even' : 'odd'; ?><?php print $data->row_class ? ' ' . $data->row_class : ''; ?>">

Just wondering why we have ($row % 2) now everywhere instead of a plain $row % 2 (without the additional brackets) as it is not needed here.

jbrown’s picture

jbrown commented
Status: Needs work » Reviewed & tested by the community

I just put that in to make it clearer. Relying on the precedence of tertiary operators is very error prone.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

PHP ternaries are backwards (left associative). The more we can do to clear up ambiguities, the better

fubhy’s picture

fubhy commented

That is true, yes, but it still wouldn't be needed here. But I don't object to it and it probably gives us better readability. So lets get it commited.

catch’s picture

catch
he/him
Primary language English
commented

This feels like a lot of copy and paste <?php print ($delta % 2) ? 'even' : 'odd'

What about a helper for that?

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Also there's no CSS changes here, do we really have no table striping CSS in core?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

There is already even and odd CSS styles, the point is just that the first row is picking up the even styles.

I don't know if a helper is really necessary, but that's just me.

catch’s picture

catch
he/him
Primary language English
commented

If we had a helper, we could've fixed this in one place though no instead of 5?

With the CSS, won't this be a visual change though? i.e. do we want to switch those around. If the answer to that is no that's fine with me, just asking ;)

jbrown’s picture

jbrown commented

It was suggested in #14 that it is better being a pattern. I think this is best. A function call would be as simple as the pattern, but with the pattern you can see what it does without having to go hunting. Also saves a function call.

The CSS should probably be switched also, otherwise tables will be starting with the wrong colour.

danquah’s picture

danquah commented
StatusFileSize
new zebra-1103340-22-reroll-32.patch4.3 KB

Patch #22 rerolled it for current 8.x HEAD ( 8deaba40a24436c3b2ecacdfe51c2f1a5a0f4f4e ).

therobyouknow’s picture

therobyouknow commented

Tried to apply patch in drupal 8 dev today and would not apply:

Robert-Daviss-MacBook-Pro:includes robertdavis$ ls
ajax.inc				entity.inc				menu.inc				theme.maintenance.inc
authorize.inc				errors.inc				module.inc				token.inc
batch.inc				file.inc				pager.inc				unicode.entities.inc
bootstrap.inc				file.mimetypes.inc			password.inc				unicode.inc
cache.inc				form.inc				path.inc				update.inc
common.inc				image.inc				schema.inc				utility.inc
config.inc				install.core.inc			session.inc				zebra-1103340-22-reroll-32.patch
database.inc				install.inc				standard.inc
date.inc				language.inc				tablesort.inc
entity.api.php				mail.inc				theme.inc
Robert-Daviss-MacBook-Pro:includes robertdavis$ git apply zebra-1103340-22-reroll-32.patch 
error: patch failed: core/includes/theme.inc:1982
error: core/includes/theme.inc: patch does not apply
Robert-Daviss-MacBook-Pro:includes robertdavis$ pwd
/Users/robertdavis/Sites/drupal/core/includes
Robert-Daviss-MacBook-Pro:includes robertdavis$
therobyouknow’s picture

therobyouknow commented
Status: Needs review » Needs work
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Could this suggestion be all done in CSS?

Remove first/last/odd/even classes in favor of CSS3 pseudo selectors
See http://drupal.org/node/1192156

mbutsko’s picture

mbutsko commented
Status: Needs work » Needs review

#32: zebra-1103340-22-reroll-32.patch queued for re-testing.

mbutsko’s picture

mbutsko commented

The patch in #32 applied for me today in 8.x HEAD { 522188692156731724b59bcfc06207b5b0d1e3ec }. Re-test.

[8.x][~/dev/d8] git apply -v zebra-1103340-22-reroll-32.patch
Checking patch core/includes/theme.inc...
Hunk #1 succeeded at 2053 (offset 71 lines).
Hunk #2 succeeded at 2074 (offset 71 lines).
Checking patch core/modules/block/templates/block-admin-display-form.tpl.php...
Checking patch core/modules/field/field.module...
Hunk #1 succeeded at 1254 (offset -19 lines).
Checking patch core/modules/field/templates/field.tpl.php...
Checking patch core/modules/forum/forum.module...
Hunk #1 succeeded at 1154 (offset -1 lines).
Hunk #2 succeeded at 1208 (offset -1 lines).
Applied patch core/includes/theme.inc cleanly.
Applied patch core/modules/block/templates/block-admin-display-form.tpl.php cleanly.
Applied patch core/modules/field/field.module cleanly.
Applied patch core/modules/field/templates/field.tpl.php cleanly.
Applied patch core/modules/forum/forum.module cleanly.
fubhy’s picture

fubhy commented
Status: Needs review » Closed (won't fix)

Yes, this is being handled over here:

#1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors

We will never commit this to D7 because of this potentially destroying existing D7 theming. So let's just close this as won't fix and instead work on #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors.