Problem/Motivation

Zebra striping can be disabled for a table with the no_striping option. When tabledrag is applied to the table and a user drags a row, tabledrag applies the odd/even classes anyway. In Seven this is particularly noticable, since tables with zebra striping have different styling.

Annotated screenshot of undragged table

Annotated screenshot of a dragged table

Proposed resolution

Remove the application of odd/even classes on tables with no_striping enabled.

Remaining tasks

  • Write a patch
  • Review

User interface changes

Table rows on a draggable table are styled consistently before and after dragging.

API changes

None

CommentFileSizeAuthor
#14 2412579-14.patch1.43 KBgeertvd
#11 interdiff.txt1.65 KBgeertvd
#11 2412579-11.patch1.5 KBgeertvd
#8 2412579-8.patch1.44 KBidebr
#8 interdiff-8-3.txt389 bytesidebr
#8 2412579-8-region-header.png54.97 KBidebr
#3 2412579-3.patch1.44 KBidebr
#1 no_striping_ignored_after_tabledrag-2412579-1.patch857 bytesgeertvd
tabledrag-dragged-before.png148.5 KBidebr
tabledrag-undragged-before.png142.95 KBidebr

Comments

geertvd’s picture

geertvd commented
Status: Active » Needs review
StatusFileSize
new no_striping_ignored_after_tabledrag-2412579-1.patch857 bytes
idebr’s picture

idebr commented
Assigned: Unassigned » idebr
Status: Needs review » Needs work

@geertvd The patch applies cleanly and solves the odd/even classes being added to tables without odd/even classes. However, I think the tabledrag functionality would perform better if it could skip calling the rewriteTable() function altogether. This could be achieved by exposing the setting as an attribute on the table and skip the function calls for tables with the setting. I'll work on a patch for this approach.

idebr’s picture

idebr commented
Status: Needs work » Needs review
StatusFileSize
new 2412579-3.patch1.44 KB

Attached patch exposes the no-striping option on the table and adds a new setting 'noStriping' in tabledrag.js. This prevents the restripeTable() from being called.

idebr’s picture

idebr commented
Assigned: idebr » Unassigned

Status: Needs review » Needs work

The last submitted patch, 3: 2412579-3.patch, failed testing.

geertvd’s picture

geertvd commented

I was thinking to fix this in the same approach but didn't since no_striping is actually a setting on a row rather then on a table.
Honestly I can't think of a use case where putting this setting on a row and not on the entire table would be useful though.

In any case at the moment there's no guarantee that $variables['no_striping'] is going to exist which will lead to undefined index notices.

geertvd’s picture

geertvd commented

#1959110: theme_table outputs the no_striping option as an HTML attribute (followups) is an interesting thread on this.

idebr’s picture

idebr commented
Status: Needs work » Needs review
StatusFileSize
new 2412579-8-region-header.png54.97 KB
new interdiff-8-3.txt389 bytes
new 2412579-8.patch1.44 KB

Interesting indeed. I did some digging to see why this is added on the row instead of the table and found #616240: Make Field UI screens extensible from contrib - part II:

API additions:
- adds a 'no_striping' option on rows in theme_table() : the "Manage display" table has 'region heading' rows, that cannot be part of the 'even/odd' striping, while theme_table() currently stripes all rows.

The selector in tabledrag.js as well the table generation in #theme => table currently makes no such distinction, so it seems safe to remove striping from the table altogether in this case:

The updated patch should fix the failing tests.

geertvd’s picture

geertvd commented
Status: Needs review » Reviewed & tested by the community

New patch fixes the undefined index notices and fixes the issue.
I also agree with removing the striping on a table level rather then a row level.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/includes/theme.inc
@@ -1042,6 +1042,9 @@ function template_preprocess_table(&$variables) {
+    $variables['attributes']['data-no-striping'] = 1;

+++ b/core/misc/tabledrag.js
@@ -60,6 +60,7 @@
+    this.noStriping = $(this.table).data('no-striping') === 1;

Let's make this stripping since this works better logically. If stripping is set to true then we need to restripe the table. Yes I know this is different to the no_striping but two wrongs don't make a right :)

geertvd’s picture

geertvd commented
StatusFileSize
new 2412579-11.patch1.5 KB
new interdiff.txt1.65 KB
geertvd’s picture

geertvd commented
Status: Needs work » Needs review
idebr’s picture

idebr commented

Thanks for picking this up, geertvd.

+++ b/core/includes/theme.inc
@@ -1042,8 +1042,8 @@ function template_preprocess_table(&$variables) {
+    $variables['attributes']['data-striping'] = 1;

This could also be written as empty(($variables['no_striping']) to make it more readable.

The other changes look good, thank you:)

geertvd’s picture

geertvd commented
StatusFileSize
new 2412579-14.patch1.43 KB

True, I thought I tried that first but found a use-case where this would not work. Must be getting tired :)

idebr’s picture

idebr commented
Status: Needs review » Reviewed & tested by the community

Re: alexpott #10

Let's make this stripping since this works better logically.

I suppose alexpott ment striping instead of stripping. The patch in #14 addresses this feedback, so back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I did mean striping :) This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 8e19910 and pushed to 8.0.x. Thanks!

  • alexpott committed 8e19910 on 8.0.x 
    Issue #2412579 by idebr, geertvd: Tabledrag applies odd/even classes to...

Status: Fixed » Closed (fixed)

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