r96509 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96508‎ | r96509 | r96510 >
Date:22:03, 7 September 2011
Author:brion
Status:ok (Comments)
Tags:
Comment:
Followup to r86108, r86854, r96384: table sorter fetch of 'data-sort-value' attribute failed on IE 6/7 due to directly using DOM methods not available in those browsers.

hasAttribute and getAttribute don't appear until IE 8 in Microsoft-land; switching to jQuery's .attr() resolves this nicely.
Modified paths:
  • /trunk/phase3/resources/jquery/jquery.tablesorter.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.tablesorter.js
——@@ -76,10 +76,12 @@
7777 }
7878
7979 function getElementText( node ) {
80 - if ( node.hasAttribute && node.hasAttribute( 'data-sort-value' ) ) {
81 - return node.getAttribute( 'data-sort-value' );
 80+ var $node = $( node ),
 81+ data = $node.attr( 'data-sort-value' );
 82+ if ( data !== undefined ) {
 83+ return data;
8284 } else {
83 - return $( node ).text();
 85+ return $node.text();
8486 }
8587 }
8688

Follow-up revisions

RevisionCommit summaryAuthorDate
r96849REL1_18: r96509, r96522, r96606, r96643, r96645, r96655, r96659, r96687, r967......reedy15:03, 12 September 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r86108Followup ro r86088: Use data-sort-type instead of classes to specify the pars...diebuche08:23, 15 April 2011
r86854Followup to r86108: jQuery tries to be too smart and converts the string to a...diebuche13:15, 25 April 2011
r96384jquery.tablesorter.test: Add tests for data-sort-value...krinkle23:09, 6 September 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   22:04, 7 September 2011

Since earlier bits are in 1.18, this fix should be applied as well.

#Comment by Krinkle (talk | contribs)   22:16, 7 September 2011

I thought it was using jQuery already. Duh! All green again :) http://toolserver.org/~krinkle/testswarm/job/355/

#Comment by Krinkle (talk | contribs)   22:21, 7 September 2011

btw, other code is using .data( 'sort-value' ). That's the way it should be accessed. For now no difference, but for the future, using attributes might not work as expected (as they are only read from on initialization to populate the .data )

#Comment by Brion VIBBER (talk | contribs)   22:22, 7 September 2011

It was changed from .data() to using attributes in r86854 to work around behavior in jQuery turning things into numbers sometimes.

#Comment by DieBuche (talk | contribs)   09:18, 14 September 2011

This will break number sorting, but 

   if ( data !== undefined ) {
       return data.toString();
    }

should fix that.

#Comment by DieBuche (talk | contribs)   09:29, 14 September 2011

Strike that, didn't see you were using .attr() instead of .data()

Status & tagging log