launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/sortable-dates-fix-918892 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #918892 in Launchpad itself: "sortable.js doesn't sort dates correctly"
  https://bugs.launchpad.net/launchpad/+bug/918892

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sortable-dates-fix-918892/+merge/89380

== Implementation ==

The third party sorttable.js needed to be patched to support tables with colspans. The patch wasn't quite done correctly. The adjustment to the column index needed to be moved up a bit so that the guessType() method, which determines whether a 
column is a string, int or date, operates on the correct column.

== Tests ==

Manually test the code index page eg code.launchpad.dev/firefox
Ensure branches are correctly sorted by Last Modified Date
-- 
https://code.launchpad.net/~wallyworld/launchpad/sortable-dates-fix-918892/+merge/89380
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sortable-dates-fix-918892 into lp:launchpad.

=== modified file 'lib/lp/app/javascript/sorttable/sorttable.js'
--- lib/lp/app/javascript/sorttable/sorttable.js	2012-01-18 06:24:25 +0000
+++ lib/lp/app/javascript/sorttable/sorttable.js	2012-01-20 05:20:30 +0000
@@ -89,12 +89,6 @@
           if (!headrow[i].className.match(/\bsorttable_nosort\b/)) { // skip this col
             mtch = headrow[i].className.match(/\bsorttable_([a-z0-9]+)\b/);
             if (mtch) { override = mtch[1]; }
-              if (mtch && typeof sorttable["sort_"+override] == 'function') {
-                headrow[i].sorttable_sortfunction = sorttable["sort_"+override];
-              } else {
-                headrow[i].sorttable_sortfunction = sorttable.guessType(table,i);
-              }
-              // Make it clickable to sort.
               // If some previous column contains a colspan, we need to
               // increase the column index to compensate for it.
               var colspan_offset = 0;
@@ -110,7 +104,13 @@
                   }
               }
               headrow[i].sorttable_columnindex = i + colspan_offset;
-
+              if (mtch && typeof sorttable["sort_"+override] == 'function') {
+                headrow[i].sorttable_sortfunction = sorttable["sort_"+override];
+              } else {
+                headrow[i].sorttable_sortfunction = sorttable.guessType(
+                  table, i + colspan_offset);
+              }
+              // Make it clickable to sort.
               headrow[i].sorttable_tbody = table.tBodies[0];
               dean_addEvent(headrow[i],"click", function(e) {