tableheader.js creates a second table which holds the sticky headers. To help ensure that the sticky headers are styled the same way as the original table's headers, the sticky-header table should have the same @class as the original table.

The attached patch patches tableheader.js so that the new table has the same classes as the original, except sticky-enabled, which is removed. sticky-header is added, as happens currently.

Files: 
CommentFileSizeAuthor
#40 590328-40.patch677 bytesdrintios
#32 interdiff-590328-30-32.txt1.12 KBzaporylie
#32 590328-32.patch677 byteszaporylie
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,158 pass(es), 37 fail(s), and 7 exception(s). View
#30 tableheader_js_should-590328-30.patch1.08 KBFreekyMage
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,984 pass(es). View
#28 Screen Shot 2015-01-17 at 15.01.33.png38.1 KBzaporylie
#28 Screen Shot 2015-01-17 at 14.28.35.png35.07 KBzaporylie
#27 tableheader_js_should-590328-27.patch1.21 KBzaporylie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,014 pass(es). View
#25 tableheader-class-590328-24.patch1.21 KBadci_contributor
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,183 pass(es). View
#17 tableheader-class.patch1.25 KBLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 42,120 pass(es). View
#16 tableheader-class.patch1.05 KBLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 37,295 pass(es). View
#12 tableheader-class.patch1.05 KBLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 34,398 pass(es). View
#11 tableheader-class.patch1.05 KBLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 34,393 pass(es). View
#10 tableheader-class.patch1.03 KBLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 29,853 pass(es). View
#9 tableheader-class-d8.patch1.03 KBLiam Morland
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tableheader-class-d8.patch. Unable to apply patch. See the log in the details link for more information. View
#6 tableheader-class.patch1.21 KBLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 18,814 pass(es). View
#3 tableheader-class.patch1.24 KBLiam Morland
Passed on all environments. View
tableheader-class.patch1.17 KBLiam Morland
Failed: Failed to apply patch. View

Comments

Liam Morland’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
Passed on all environments. View

Updated patch.

Liam Morland requested that failed test be re-tested.

aspilicious’s picture

Status: Needs review » Fixed

current code

      headerClone = $(headerClone)[0];

      // Store parent table.
      var table = $(this).parent('table')[0];
      headerClone.table = table;
      // Finish initializing header positioning.
      tracker(headerClone);
      // We hid the header to avoid it showing up erroneously on page load;
      // we need to unhide it now so that it will show up when expected.
      $(headerClone).children('thead').show();

      $(table).addClass('sticky-table');

Means sticky_table class is set :)!
Fixed!

Liam Morland’s picture

Status: Fixed » Needs review
FileSize
1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 18,814 pass(es). View

The point if this patch is to ensure that the new table containing the sticky header has the same class as the original table, minus sticky-enabled, plus sticky-table. This is needed because some modules create sticky-enabled tables with other classes assigned. Without this patch, the sticky header tables won't match up with their tables since they have different CSS rules applied to them. I have attached my patch rerolled against the latest D7.

Status: Needs review » Needs work

The last submitted patch, tableheader-class.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#6: tableheader-class.patch queued for re-testing.

Liam Morland’s picture

Version: 7.x-dev » 8.x-dev
FileSize
1.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tableheader-class-d8.patch. Unable to apply patch. See the log in the details link for more information. View

Attached is a version of this patch rolled against the latest Drupal 8.

Liam Morland’s picture

FileSize
1.03 KB
PASSED: [[SimpleTest]]: [MySQL] 29,853 pass(es). View

Same patch again, renamed so that it will get tested.

Liam Morland’s picture

FileSize
1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 34,393 pass(es). View

Updated patch rolled against latest D8.

Liam Morland’s picture

FileSize
1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 34,398 pass(es). View

Sorry, typo. Updated patch rolled against latest D8.

Status: Needs review » Needs work

The last submitted patch, tableheader-class.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review

#11: tableheader-class.patch queued for re-testing.

Liam Morland’s picture

#12: tableheader-class.patch queued for re-testing.

Liam Morland’s picture

FileSize
1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 37,295 pass(es). View

Updated patch rolled against latest D8.

Liam Morland’s picture

FileSize
1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 42,120 pass(es). View

Reroll.

Kiphaas7’s picture

+    // Copy classes from originalTable, remove undesired classes, and add
+    // sticky-header. Any other classes added to originalTable by modules will
+    // exists in stickyTable to ensure consistent styling.
+    this.$stickyTable.attr('class', this.originalTable.attr('class'));
+    this.$stickyTable.removeClass('sticky-enabled tableheader-processed sticky-table');
+    this.$stickyTable.addClass('sticky-header');

This looks fragile to me: suppose we're at a table with tabledrag enabled, or tableselect. They both add "-processed" classes, and both wouldn't be desired to be on the sticky table. let alone any contrib module.

Either we need a good whitelist (instead of blacklisting classes), or we need to pass the CSS classes set in php as a setting?

Liam Morland’s picture

Does it hurt if the sticky header has these extra classes? The idea of this patch is to ensure that the sticky header has the same classes so that it is sure to be styled the same.

droplet’s picture

@Kiphaas7,
can you explain more ? i don't get your meaning :)

@Liam,
No hurts I think and it should be.

droplet’s picture

Issue summary: View changes
Issue tags: +JavaScript
Kiphaas7’s picture

I can't remember what my actual problem was there, but I think I was complaining about the blacklisting (=removing) of _just_ the classes mentioned in

this.$stickyTable.removeClass('sticky-enabled tableheader-processed sticky-table');

Contrib might do funky stuff to a table or table header as well, which might not be wanted on the sticky header. If the classes which are to be deleted are a javascript inline setting (passed in by php) contrib can overrule/extend it.

… Or contrib should write extra js to manually remove the class itself. However, that smells like a lot of event duplication in the javascript code. Basically I'd prefer if it was something like this

this.$stickyTable.removeClass(some.inline.setting.string.containing.all.the.classes);

Liam Morland’s picture

If I understand correctly, you would like modules to be able to add to the list of classes that are removed. That makes sense to me.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
adci_contributor’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,183 pass(es). View

Rerolled

zaporylie’s picture

Assigned: Unassigned » zaporylie
Status: Needs review » Needs work
Issue tags: +SprintWeekend2015

Because of missing `$` this patch doesn't work and gives errors in log console.

+++ b/core/misc/tableheader.js
@@ -158,6 +158,13 @@
+      this.$stickyTable.attr('class', this.originalTable.attr('class'));

Should be:

+++ b/core/misc/tableheader.js
@@ -158,6 +158,13 @@
+      this.$stickyTable.attr('class', this.$originalTable.attr('class'));

I will fix #25 and provide a patch in next comment.

zaporylie’s picture

Assigned: zaporylie » Unassigned
Status: Needs work » Needs review
FileSize
1.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,014 pass(es). View

...and here it is.

zaporylie’s picture

This is clean D8:

<table class="sticky-header" style="visibility: visible; position: fixed; top: 39px; left: 272.515625px; width: 1039px;">...</table>
<table class="views-table views-view-table sticky-enabled cols-7 responsive-enabled tableresponsive-processed table-select-processed tableheader-processed sticky-table">...</table>

and this is D8 + patch #27:

<table class="views-table views-view-table cols-7 responsive-enabled tableresponsive-processed table-select-processed sticky-header" style="visibility: hidden; position: fixed; top: 39px; left: 272.515625px; width: 1039px;">...</table>
<table class="views-table views-view-table sticky-enabled cols-7 responsive-enabled tableresponsive-processed table-select-processed tableheader-processed sticky-table">...</table>

I attached screenshots from chrome inspector as well.

Wim Leers’s picture

Issue tags: +markup, +CSS
FreekyMage’s picture

FileSize
1.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,984 pass(es). View

Reviewed, but changed so that the code is chained, which should give a small performance boost.

nod_’s picture

Status: Needs review » Needs work
+++ b/core/misc/tableheader.js
@@ -149,12 +149,18 @@
+        .attr('class', this.$originalTable.attr('class'))

Use addClass for consistency.

Also why do we do document.createElement now? we can just keep that and get rid of the addClass("sticky-header") line.

zaporylie’s picture

Status: Needs work » Needs review
FileSize
677 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,158 pass(es), 37 fail(s), and 7 exception(s). View
1.12 KB

Here's a patch and interdiff.

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

Applies, sticky table inherits same classes as main table.

Kiphaas7’s picture

And the comments in #18 - #23 are out of scope, or do these concerns no longer apply?

Liam Morland’s picture

Also why do we do document.createElement now?

I thought that would be neater and more clear, because it keeps all the class-related code together. As well, I suspect that it runs faster since jQuery no longer has to parse an HTML table tag with @class.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 590328-32.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

drintios’s picture

Assigned: Unassigned » drintios
drintios’s picture

Did reroll of patch #32 against 8.2.x

drintios’s picture

Status: Needs work » Needs review

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

Liam Morland’s picture

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