r87322 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r87321‎ | r87322 | r87323 >
Date:11:44, 3 May 2011
Author:diebuche
Status:ok (Comments)
Tags:todo 
Comment:
Tablesorter: Add a title attribute to sort arrows ( Bug 21453 )
Modified paths:
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.tablesorter.js (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
@@ -338,6 +338,9 @@
339339 'anonnotice',
340340 'newsectionheaderdefaultlevel',
341341 'red-link-title',
 342+ 'sort-descending',
 343+ 'sort-ascending',
 344+
342345 ),
343346 'nstab' => array(
344347 'nstab-main',
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -933,6 +933,8 @@
934934 'anonnotice' => '-', # do not translate or duplicate this message to other languages
935935 'newsectionheaderdefaultlevel' => '== $1 ==', # do not translate or duplicate this message to other languages
936936 'red-link-title' => '$1 (page does not exist)',
 937+'sort-descending' => 'Sort descending',
 938+'sort-ascending' => 'Sort ascending',
937939
938940 # Short words for each namespace, by default used in the namespace tab in monobook
939941 'nstab-main' => 'Page',
Index: trunk/phase3/resources/jquery/jquery.tablesorter.js
@@ -141,6 +141,7 @@
142142 // Check next parser, reset rows
143143 i++;
144144 rowIndex = 0;
 145+ concurrent = 0;
145146 }
146147 } else {
147148 // Empty cell
@@ -255,7 +256,7 @@
256257 // }
257258 }
258259
259 - function buildHeaders( table ) {
 260+ function buildHeaders( table, msg ) {
260261 var maxSeen = 0;
261262 var longest;
262263 // if ( table.config.debug ) {
@@ -289,7 +290,8 @@
290291 if ( $( this ).is( '.unsortable' ) ) this.sortDisabled = true;
291292
292293 if ( !this.sortDisabled ) {
293 - var $th = $( this ).addClass( table.config.cssHeader );
 294+ var $th = $( this ).addClass( table.config.cssHeader ).attr( 'title', msg[1] );
 295+
294296 //if ( table.config.onRenderHeader ) table.config.onRenderHeader.apply($th);
295297 }
296298
@@ -316,7 +318,7 @@
317319 return false;
318320 }
319321
320 - function setHeadersCss( table, $headers, list, css ) {
 322+ function setHeadersCss( table, $headers, list, css, msg ) {
321323 // remove all header information
322324 $headers.removeClass( css[0] ).removeClass( css[1] );
323325
@@ -329,7 +331,7 @@
330332
331333 var l = list.length;
332334 for ( var i = 0; i < l; i++ ) {
333 - h[list[i][0]].addClass( css[list[i][1]] );
 335+ h[ list[i][0] ].addClass( css[ list[i][1] ] ).attr( 'title', msg[ list[i][1] ] );
334336 }
335337 }
336338
@@ -515,8 +517,13 @@
516518 $this = $( this );
517519 // save the settings where they read
518520 $.data( this, "tablesorter", config );
 521+
 522+ // get the css class names, could be done else where.
 523+ var sortCSS = [ config.cssDesc, config.cssAsc ];
 524+ var sortMsg = [ mw.msg( 'sort-descending' ), mw.msg( 'sort-ascending' ) ];
 525+
519526 // build headers
520 - $headers = buildHeaders( this );
 527+ $headers = buildHeaders( this, sortMsg );
521528 // Grab and process locale settings
522529 buildTransformTable();
523530 buildDateTable();
@@ -526,8 +533,6 @@
527534 //performance improvements in some browsers
528535 cacheRegexs();
529536
530 - // get the css class names, could be done else where.
531 - var sortCSS = [config.cssDesc, config.cssAsc];
532537 // apply event handling to headers
533538 // this is to big, perhaps break it out?
534539 $headers.click(
@@ -584,7 +589,7 @@
585590 }
586591 setTimeout( function () {
587592 // set css for headers
588 - setHeadersCss( $this[0], $headers, config.sortList, sortCSS );
 593+ setHeadersCss( $this[0], $headers, config.sortList, sortCSS, sortMsg );
589594 appendToTable(
590595 $this[0], multisort(
591596 $this[0], config.sortList, cache ) );
Index: trunk/phase3/resources/Resources.php
@@ -137,7 +137,8 @@
138138 'scripts' => 'resources/jquery/jquery.tabIndex.js',
139139 ),
140140 'jquery.tablesorter' => array(
141 - 'scripts' => 'resources/jquery/jquery.tablesorter.js'
 141+ 'scripts' => 'resources/jquery/jquery.tablesorter.js',
 142+ 'messages' => array( 'sort-descending', 'sort-ascending' ),
142143 ),
143144 'jquery.textSelection' => array(
144145 'scripts' => 'resources/jquery/jquery.textSelection.js',

Sign-offs

UserFlagDate
Krinkleinspected03:35, 29 May 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r86088Completely rewritten table sorting script....diebuche21:47, 14 April 2011

Comments

#Comment by Krinkle (talk | contribs)   08:35, 14 May 2011

+							concurrent = 0;

What is that for ?

#Comment by DieBuche (talk | contribs)   09:27, 14 May 2011

That's a mini followup to r87243 . We walk trough multiple cells to find the parser that matches best.

#Comment by Brion VIBBER (talk | contribs)   20:33, 7 June 2011

Honestly I'm a bit leery of this; the appearance of the arrow shows the current state, but the tooltip describes the state after the next action?

It's unclear just from looking at it whether "Sort ascending" means "This table is sorted ascending" or "Change the sort on this table to ascending on this column"

#Comment by DieBuche (talk | contribs)   21:11, 7 June 2011

I think we both agree on the current behaviour of the arrows themselves (show current state) and imho a "link" tooltip should show the action that's done when clicking it. Maybe just clarify the string to "Click to sort ascending" (even though "click here" seems to be discouraged..). I find "Change the sort on this table to ascending on this column" a bit to wordy

#Comment by Nikerabbit (talk | contribs)   21:13, 7 June 2011

I don't think it is that confusing. If the tooltip would describe the current state it would probably say "sorted ascending" or somesuch.

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

nod it's not superawful; taking this off fixme, leaving a todo tag.

Status & tagging log