r97150 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r97149‎ | r97150 | r97151 >
Date:13:15, 15 September 2011
Author:catrope
Status:ok (Comments)
Tags:needs-js-test 
Comment:
Update jquery.tablesorter for r97145: emulate <thead> if there is no <thead> in the HTML, by walking down the table starting at the first row and moving rows to the <thead> as long as all of its cells are <th>s (or the row is empty). Also fix and simplify the sortbottom code, which was incorrectly creating multiple <tfoot> elements if there were multiple sortbottom rows.
Modified paths:
  • /trunk/phase3/resources/jquery/jquery.tablesorter.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.tablesorter.js
@@ -218,6 +218,26 @@
219219 }
220220 table.tBodies[0].appendChild( fragment );
221221 }
 222+
 223+ /**
 224+ * Find all header rows in a thead-less table and put them in a <thead> tag.
 225+ * This only treats a row as a header row if it contains only <th>s (no <td>s)
 226+ * and if it is preceded entirely by header rows. The algorithm stops when
 227+ * it encounters the first non-header row.
 228+ * @param $table jQuery object for a <table>
 229+ */
 230+ function emulateTHead( $table ) {
 231+ var $thead = $( '<thead>' );
 232+ $table.find( 'tr' ).each( function() {
 233+ if ( $(this).find( 'td' ).length > 0 ) {
 234+ // This row contains a <td>, so it's not a header row
 235+ // Stop here
 236+ return false;
 237+ }
 238+ $thead.append( this );
 239+ } );
 240+ $table.prepend( $thead );
 241+ }
222242
223243 function buildHeaders( table, msg ) {
224244 var maxSeen = 0,
@@ -500,17 +520,27 @@
501521 */
502522 construct: function( $tables, settings ) {
503523 return $tables.each( function( i, table ) {
504 -
505 - // Quit if no thead or tbody.
506 - if ( !table.tHead || !table.tBodies ) {
507 - return;
508 - }
509524 // Declare and cache.
510525 var $document, $headers, cache, config, sortOrder,
511526 $table = $( table ),
512527 shiftDown = 0,
513528 firstTime = true;
514529
 530+ // Quit if no tbody
 531+ if ( !table.tBodies ) {
 532+ return;
 533+ }
 534+ if ( !table.tHead ) {
 535+ // No thead found. Look for rows with <th>s and
 536+ // move them into a <thead> tag
 537+ emulateTHead( $table );
 538+
 539+ // Still no thead? Then quit
 540+ if ( !table.tHead ) {
 541+ return;
 542+ }
 543+ }
 544+
515545 // New config object.
516546 table.config = {};
517547
@@ -545,9 +575,11 @@
546576
547577 // Legacy fix of .sortbottoms
548578 // Wrap them inside inside a tfoot (because that's what they actually want to be) &
549 - // Move them up one level in the DOM
550 - var sortbottoms = $table.find('tr.sortbottom').wrap('<tfoot>');
551 - sortbottoms.parents('table').append(sortbottoms.parent());
 579+ // and put the <tfoot> at the end of the <table>
 580+ var $sortbottoms = $table.find( 'tr.sortbottom' );
 581+ if ( $sortbottoms.length ) {
 582+ $table.append( $( '<tfoot>' ).append( $sortbottoms ) )
 583+ }
552584
553585 explodeRowspans( $table );
554586 // try to auto detect column type, and store in tables config

Sign-offs

UserFlagDate
MarkAHershbergertested14:24, 16 September 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r97173Merged revisions 97087,97091-97092,97094,97096-97098,97100-97101,97103,97136,...dantman16:19, 15 September 2011
r97378Followup to r97150 per CR: use .children() instead of .find(), more efficient...catrope15:52, 17 September 2011
r97459REL1_18 MFT r97145, r97150, r97378...reedy08:21, 19 September 2011
r101417jquery.tablesorter: Selector fixes...krinkle22:12, 31 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r97145Reverted r85922 and related: new doTableStuff(). I copied in the old doTableS...tstarling12:10, 15 September 2011

Comments

#Comment by Dantman (talk | contribs)   15:40, 16 September 2011

$(this).find( 'td' ).length > 0 using `$(this).children( 'td' ).length > 0` would be more efficient and also avoid any bugs where a th in the first row had a table or whatnot.

#Comment by Catrope (talk | contribs)   15:53, 17 September 2011

Good catch, fixed in r97378.

#Comment by MarkAHershberger (talk | contribs)   15:46, 16 September 2011

From IRC, Dantman says:

+ if ( $(this).find( 'td' ).length > 0 ) {

Using "(this).children( 'td' ).length > 0" would be more efficient and also avoid any bugs where a th in the first row had a table or whatnot.

#Comment by MarkAHershberger (talk | contribs)   15:46, 16 September 2011

oops, should have reloaded

#Comment by Krinkle (talk | contribs)   16:41, 17 September 2011

Also needs testing, last time I tried a table without a THEAD will not work properly with sortable. Would be nice to have that. Then I found this rev and turns out it doesn't work (atleast not for me, so let's build in a unit test)

Bug:


+		$table.find( 'tr' ).each( function() {
+			if ( $(this).find( 'td' ).length > 0 ) {
+				// This row contains a <td>, so it's not a header row
+				// Stop here
+				return false;
+			}
+			$thead.append( this );
+		} );

Returning false from the each callback aborts the loop, which is likely not what you intend. See also docs.

#Comment by Dantman (talk | contribs)   16:45, 17 September 2011

Really? Looks right to me.

  • Loop over each table row
  • Put any table row with just th's into the thead
  • Once you hit a row that contains a td, abort since the thead is finished.
#Comment by Krinkle (talk | contribs)   18:48, 17 September 2011

Okay, I guess it depends on what the criteria are. Not allowing further rows to be added makes sense, yes. (since that would change the visual appearance). Either way, it still needs a unit test to verify that it works.

#Comment by Catrope (talk | contribs)   13:29, 18 September 2011

Hmm, I guess inserting an empty thead might be weird. How does this not work for you?

#Comment by Krinkle (talk | contribs)   13:38, 18 September 2011

It's acceptable imho that tables that have no rows containing ‎<th> should simply not become sortable. A quick test confirmed that it's working fine for other cases.

Marking OK.

#Comment by Fomafix (talk | contribs)   21:54, 31 October 2011

$table.prepend( $thead ) puts thead as first element before caption: Bug 32047

$table.find( 'tr' ) finds table rows from subtables: Bug 32049

#Comment by Fomafix (talk | contribs)   14:47, 3 November 2011

Bug 32049 is fixed by r101417 and r101420.

Bug 32047 is still open. It can be fixed by inserting thead before tbody instead of inserting as first child of table.

Status & tagging log