tablesorter should provide a way to have a particular column be sorted in reverse order by default
OpenPublic

Description

Author: mhorvath2161

Description:
unified diff file

Add an optional table header class that allows a particular column to be sorted in reverse order by default.

Some tables (such as this one: http://en.wikipedia.org/wiki/List_of_musical_intervals) are not meaningful when sorted in ascending order. The changes allow a person to specify that a column is to be sorted in descending order by default, saving a few clicks of the mouse.

Another change was to make the ALT text for the sorting icon more verbose.


Version: unspecified
Severity: enhancement

attachment wikibits_reverse_sorting.diff ignored as obsolete

bzimport added a project: MediaWiki-JavaScript.Via ConduitNov 21 2014, 10:22 PM
bzimport added a subscriber: Unknown Object (MLST).
bzimport set Reference to bz15403.
bzimport created this task.Via LegacySep 1 2008, 2:25 AM
DanielFriesen added a comment.Via ConduitSep 1 2008, 8:34 AM

I don't like that "more verbose" alt text. We use a simple ↓ on purpose.

Alt text displays on the page when the image cannot be found. Your "Sort in ascending order." and "Sort in descending order." is quite large. In fact, far larger than 90% of the header columns that you will find in tables.

Did you even attempt to look at what the page output was when the browser could not find the image? If you bother to, I expect that you will see that this stretches header columns and makes them almost impossible to read.

bzimport added a comment.Via ConduitSep 1 2008, 3:53 PM

mhorvath2161 wrote:

(In reply to comment #1)

I don't like that "more verbose" alt text. We use a simple ↓ on purpose.
Alt text displays on the page when the image cannot be found. Your "Sort in
ascending order." and "Sort in descending order." is quite large. In fact, far
larger than 90% of the header columns that you will find in tables.
Did you even attempt to look at what the page output was when the browser could
not find the image? If you bother to, I expect that you will see that this
stretches header columns and makes them almost impossible to read.

In IE, character entities are not supported as ALT text. It results in a square being rendered instead.

bzimport added a comment.Via ConduitSep 1 2008, 7:47 PM

ayg wrote:

  • // We have a first row: assume it's the header, and make its contents clickable links
  • for (var i = 0; i < firstRow.cells.length; i++) {
  • var cell = firstRow.cells[i];
  • if ((" "+cell.className+" ").indexOf(" unsortable ") == -1) {
  • cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow"><img src="'+ ts_image_path + ts_image_none + '" alt="&darr;"/></span></a>';
  • }
  • }

+ // We have a first row: assume it's the header, and make its contents clickable links
+ for (var i = firstRow.cells.length - 1; i > -1; i--) {
+ var cell = firstRow.cells[i];
+ var cellClass = " " + cell.className + " ";
+ if (cellClass.indexOf(" unsortable ") == -1) {
+ if (cellClass.indexOf(" sortreverse ") == -1)
+ cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="up"><img src="'+ ts_image_path + ts_image_none + '" alt="Sort in ascending order."/></span></a>';
+ else
+ cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="down"><img src="'+ ts_image_path + ts_image_none + '" alt="Sort in descending order."/></span></a>';
+ }
+ }

Note, you're changing tabs to spaces here. Don't do this. It goes against our coding conventions, and also adds lines to the diff with only whitespace changes. (I've manually removed that change in my comments below for readability.) You've also included changes from your other patches that aren't relevant here, like changing the loop to run backwards, and removing some braces.

The alternate text is also much too long, as Daniel says. If &darr; doesn't work on IE, please file that as a separate bug.

  • var arrowHTML; if (reverse) {
  • arrowHTML = '<img src="'+ ts_image_path + ts_image_down + '" alt="&darr;"/>';
  • newRows.reverse();
  • span.setAttribute('sortdir','up');

+ span.innerHTML = '<img src="' + ts_image_path + ts_image_down + '" alt="Sort in ascending order."/>';
+ span.setAttribute('sortdir', 'up');
+ newRows.reverse();

	} else {
  • arrowHTML = '<img src="'+ ts_image_path + ts_image_up + '" alt="&uarr;"/>';
  • span.setAttribute('sortdir','down');

+ span.innerHTML = '<img src="' + ts_image_path + ts_image_up + '" alt="Sort in descending order."/>';
+ span.setAttribute('sortdir', 'down');

	}

See note above about the alt text. Does this change serve any other purpose? What does the removal of arrowHTML actually do?

I haven't looked at the rest of the patch yet (i.e., the important part).

bzimport added a comment.Via ConduitSep 1 2008, 11:28 PM

mhorvath2161 wrote:

(In reply to comment #3)

See note above about the alt text. Does this change serve any other purpose?
What does the removal of arrowHTML actually do?
I haven't looked at the rest of the patch yet (i.e., the important part).

In the original version, the variable arrowHTML is used to "undo" a change done in the previous loop (i.e., the loop does something to all items in an array, arrowHTML is used to "undo" the change to one of those items).

The modified code does additional changes within that loop, and simply "undoing" it wasn't workable. Instead, I placed a check in the loop to not do anything to the single item.

bzimport added a comment.Via ConduitSep 14 2008, 6:36 PM

ayg wrote:

Please fix the issues that were identified above -- changing alt text, changing tabs to spaces, changing loops to run backwards, adding lines that don't end with semicolons -- so I can more easily review this. For the time being I'll look at some of your other patches. (I'm trying to do an average of about one patch review per day right now, but it's split up between you and a couple of other people.)

bzimport added a comment.Via ConduitSep 18 2008, 5:16 AM

mhorvath2161 wrote:

unified diff file

Update, minus other issues.

Attached: wikibits_reverse_sorting.diff

bzimport added a comment.Via ConduitOct 8 2008, 6:49 PM

ayg wrote:

  • if ((" "+cell.className+" ").indexOf(" unsortable ") == -1) {
  • cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow"><img src="'+ ts_image_path + ts_image_none + '" alt="&darr;"/></span></a>';

+ var cellClass = " " + cell.className + " ";
+ if (cellClass.indexOf(" unsortable ") == -1) {
+ if (cellClass.indexOf(" sortreverse ") == -1)
+ cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="up"><img src="'+ ts_image_path + ts_image_none + '" alt="&uarr;"/></span></a>';
+ else
+ cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="down"><img src="'+ ts_image_path + ts_image_none + '" alt="&darr;"/></span></a>';

The indentation here is pretty wacky. You should indent new control structures. Also, there's a lot of repetition, and it takes a few seconds to spot the differences. Why not something more like:

		var cellClass = " " + cell.className + " ";
		if (cellClass.indexOf(" unsortable ") == -1) {
			var arrowDir = cellClass.indexOf(" sortreverse ") == -1 ? "up" : "down";
			cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="'
				+ arrowDir + '"><img src="'+ ts_image_path + ts_image_none
				+ '" alt="&' + arrowDir[0] + 'arr;"/></span></a>';
		}
  • spans[i].innerHTML = '<img src="'+ ts_image_path + ts_image_none + '" alt="&darr;"/>';

+ var thisSpan = spans[i]
+ if (thisSpan != span) {
+ var cellClass = " " + thisSpan.parentNode.parentNode.className + " "
+ if (cellClass.indexOf(" sortreverse ") == -1) {
+ thisSpan.setAttribute("sortdir", "up")
+ thisSpan.innerHTML = '<img src="' + ts_image_path + ts_image_none + '" alt="Sort in ascending order."/>'
+ } else {
+ thisSpan.setAttribute("sortdir", "down")
+ thisSpan.innerHTML = '<img src="' + ts_image_path + ts_image_none + '" alt="Sort in descending order."/>'
+ }
+ }

  1. You're missing semicolons here.
  1. As discussed, alt text like "Sort in descending order" is way too long. It will stretch out the table horribly. Try viewing the page with images disabled to determine appropriate alt text. An up/down arrow should be used, as it is now (if you think this is bad, again, file a new bug).
  1. As above, you should be able to simplify this code by having a variable that equals "up" or "down" depending on the presence of the class.
bzimport added a comment.Via ConduitDec 27 2008, 6:28 AM

mhorvath2161 wrote:

(In reply to comment #1)

I don't like that "more verbose" alt text. We use a simple &darr; on purpose.
Alt text displays on the page when the image cannot be found. Your "Sort in
ascending order." and "Sort in descending order." is quite large. In fact, far
larger than 90% of the header columns that you will find in tables.
Did you even attempt to look at what the page output was when the browser could
not find the image? If you bother to, I expect that you will see that this
stretches header columns and makes them almost impossible to read.

If the image dimensions are specified explicitely via CSS, then the length of the ALT text will have zero effect on the table size.

-Mike

bzimport added a comment.Via ConduitDec 27 2008, 11:29 PM

ayg wrote:

If the image dimensions are given via CSS, and this actually stops things from stretching, then your alt text will be completely unreadable, since it will be crammed into a tiny box. This question is orthogonal to what's being discussed in the present bug, however, and you should start a new bug to discuss it if you want it changed.

bzimport added a comment.Via ConduitDec 28 2008, 8:37 AM

mhorvath2161 wrote:

Again, you know nothing but decide to interfere anyway.

Go away.

Catrope added a comment.Via ConduitJan 7 2009, 2:34 PM

(In reply to comment #10)

Again, you know nothing but decide to interfere anyway.

Go away.

It's interesting how your implicit claim about not being rude in bug 15402 comment #22 contrasts with this comment.

Aryeh is right that discussions about changing the ALT text has little to do with reverse sorting. Since the reverse sorting part is a lot less controversial that changing the ALT text, it's probably wiser to file a separate bug for the latter, so the former can be resolved more smoothly. Also, discussions about two different things at the same time are generally discouraged around here.

bzimport added a comment.Via ConduitJan 11 2009, 2:02 AM

mhorvath2161 wrote:

(In reply to comment #11)

It's interesting how your implicit claim about not being rude in bug 15402
comment #22 contrasts with this comment.

Please don't cherry pick response to make a point, and please stay on topic.

DieBuche added a comment.Via ConduitApr 27 2011, 8:45 AM

Needs updated patch

bzimport added a comment.Via ConduitNov 4 2011, 9:00 PM

sumanah wrote:

This patch no longer applies cleanly to trunk. SharkD, could you revise so it applies to current trunk? You may also want to grab MediaWiki 1.18 beta 1 ( http://lists.wikimedia.org/pipermail/mediawiki-announce/2011-November/thread.html ) to see whether current behavior obviates the need for a fix.

Thanks for contributing to MediaWiki.

Catrope added a comment.Via ConduitNov 8 2011, 11:23 AM

(In reply to comment #14)

This patch no longer applies cleanly to trunk. SharkD, could you revise so it
applies to current trunk? You may also want to grab MediaWiki 1.18 beta 1 (
http://lists.wikimedia.org/pipermail/mediawiki-announce/2011-November/thread.html
) to see whether current behavior obviates the need for a fix.

Thanks for contributing to MediaWiki.

Note that the sortable table code has been completely rewritten, it's now in resources/jquery/jquery.tablesorter.js . I'm not sure whether the new table sorter has this feature, but if it does, the patch would probably have to be rewritten completely as well because the code it's patching doesn't exist any more.

matmarex added a comment.Via ConduitMar 10 2013, 7:51 AM

[Removing 'patch' keyword as it no longer applies to anything.]

Add Comment