Page MenuHomePhabricator

Sortable tables: Force sortorder of a column
Closed, ResolvedPublic

Description

Author: mhorvath2161

Description:
unified diff file

This patch adds the ability to force a particular sortorder of a column instead of relying on the script to detect the proper sortorder. This is achieved by adding a "sortorder" attribute to the column header, and specifying the desired sortorder as the value of this attribute. The actual options, however, need to be updated to be current with the other patches. The ones included in the diff can be considered examples used more for demonstration purposes.


Version: unspecified
Severity: enhancement
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=31332

attachment wikibits_force_sortorder.diff ignored as obsolete

Details

Reference
bz15406

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:22 PM
bzimport set Reference to bz15406.
bzimport added a subscriber: Unknown Object (MLST).

sortorder is not a valid XHTML attribute. Adding this patch will go against our attempts at keeping MediaWiki XHTML valid.

I recommend finding a way to do this using classes rather than invalid XHTML attributes.

mhorvath2161 wrote:

(In reply to comment #1)

sortorder is not a valid XHTML attribute. Adding this patch will go against our
attempts at keeping MediaWiki XHTML valid.
I recommend finding a way to do this using classes rather than invalid XHTML
attributes.

The proposal could certainly be changed to use classes. However, the code already uses a non standard XHTML attribute, "sortdir". Should this be changed as well?

Sortdir is an attribute set internally by the js when you click on the sort button. It's ok for JS to set non-standard attributes.

There is another proposal for a default reverse sort, that patch should probably make sure to be one that sets that attribute.

mhorvath2161 wrote:

It's silly to say that non-standard attributes are supported when added by JavaScript, but not otherwise. You're confusing being able to pass a verification test with the issue behind the proviso.

ayg wrote:

The sortdir thing should be changed too. Frankly I don't see why authors shouldn't be allowed to make up their own site-specific attributes. It's handy sometimes. But we want the trendy Standards-Compliant label, I suppose, so we have to live with it.

There's no way, in any event, for users to add a custom attribute in wikitext. Something like this:

{| class="sortable"

-

! sortorder="numeric" | This is the header
...

}

will just have the "sortorder" attribute stripped as invalid. So I'm not clear on how this is supposed to be used, even if we did want to break XHTML validity, which we don't.

It's only used internally. Attributes in the code are commonly used for storing variables related to a node. In this case we store the current sort direction so that we can reverse it on the next sort.

I can't remember if we have a class to make the list initially sorted... If we don't it would be good. Either way a reversesort css class to make this sort in the opposite direction by default would be nice.

mhorvath2161 wrote:

I took a look at the code to see how easy it would be to accomplish this using only class names. While it could be done, it would require supporting a separate classname for each sorting function. As the number of sorting functions increases, so does the number of classes you have to check for (as opposed to checking for a single, uniquely named attribute). I'm worried that the large number of classes will negatively affect the speed of the script.

You don't have to check for a pile of stuff, just a pattern.

var sortorder

Ack, wtf, I write a section of JS code, and bugzilla deletes it. Bah...

Well, really you'd just break up the className by \s and then test for any classNames that start with something like sortorder-

mhorvath2161 wrote:

You mean, like:

"sortorder-numeric"
"sortorder-alphanumeric"
"sortorder-date1"
...

I suppose that would work.

mhorvath2161 wrote:

unified diff file

I created an alternate version that uses class names instead of attributes. Seems to work OK.

attachment wikibits_force_sortorder_b.diff ignored as obsolete

mhorvath2161 wrote:

Actually, "sortorder-XXXX" could be truncated to "sort-XXXX" without any problems.

mhorvath2161 wrote:

unified diff file

"sortorder-xxxx" classes renamed to "sort-xxxx".

Attached:

ssanbeg wrote:

Should the regex be anchored? I think something like
var sortClass = ('' + td.className + '').match(/^sort-\w+$/);

would be more appropriate

ssanbeg wrote:

(In reply to comment #14)

Should the regex be anchored? I think something like
var sortClass = ('' + td.className + '').match(/^sort-\w+$/);

would be more appropriate

Oops, that would probably break multiple classes....

mhorvath2161 wrote:

(In reply to comment #15)

(In reply to comment #14)

Should the regex be anchored? I think something like
var sortClass = ('' + td.className + '').match(/^sort-\w+$/);

would be more appropriate

Oops, that would probably break multiple classes....

You just made me notice that the quotes are supposed to have spaces in them. I.e.

  • var sortClass = ('' + td.className + '').match(/(sort\-\w+)/);

+ var sortClass = (' ' + td.className + ' ').match(/(sort\-\w+)/);

-Mike

ssanbeg wrote:

(In reply to comment #16)

(In reply to comment #15)

(In reply to comment #14)

Should the regex be anchored? I think something like
var sortClass = ('' + td.className + '').match(/^sort-\w+$/);

would be more appropriate

Oops, that would probably break multiple classes....

You just made me notice that the quotes are supposed to have spaces in them.
I.e.

  • var sortClass = ('' + td.className + '').match(/(sort\-\w+)/);

+ var sortClass = (' ' + td.className + ' ').match(/(sort\-\w+)/);

-Mike

Then should the regex have the spaces, too? i.e.

var sortClass = (' ' + td.className + ' ').match(/ (sort\-\w+) /);

It seems like that should avoid false positives in the match.

david.potter wrote:

*** Bug 13535 has been marked as a duplicate of this bug. ***

mhorvath2161 wrote:

(In reply to comment #17)

Then should the regex have the spaces, too? i.e.

var sortClass = (' ' + td.className + ' ').match(/ (sort\-\w+) /);

It seems like that should avoid false positives in the match.

I suppose it can't hurt. I don't think false positives are very likely though.

-Mike

bluehairedlawyer wrote:

How come this has stalled for so long?

will be fixed in the jQuery implementation

mhorvath2161 wrote:

Quote: "How come this has stalled for so long?"

Because I got into an argument with the devs and they said they would no longer be looking at my requests.

r86088 is seriously broken and may get reverted; reopening. Needs unit tests (see initial basic unit tests in r90595 which show up errors; will also need tests for the particular case this bug is about.)

This needs to be implemented soon. It is very difficult to follow all the rules in this and other sections of the Help:Sorting page. Even experienced table editors like myself have difficulty. There are so many rules, and they change at times:
*http://en.wikipedia.org/wiki/Help:Sorting#Numerical_sorting_problems

I have been updating Help:Sorting, and doing various tests. There are still some inexplicable bugs where a column follows all the rules for numerical sorting, and it still does not sort correctly. I have looked at r86088 comments, and I am glad there has been some serious work down.

Has work on this stalled? If you are working on this then please also incorporate my request to put the sorting icon above the text. This is very important. See my comment #15 here:
*https://bugzilla.wikimedia.org/show_bug.cgi?id=31755#c15

See this related feature request:
Bug 24523 - "Make table sorting ignore refs when determining data type".

This has been fixed in English Wikipedia according to discussion here:
*[[Help_talk:Sorting#HTML5]]
*[[Help_talk:Sorting#Documentation]]

data-sort-type plus a value forces a column to be sorted according to a specific method.

See also:
*[[Help:Sorting#Numerical_sorting_problems]]
*[[meta:Help:Sorting#Sort_modes]] - go to the subsection about forcing the sort mode for a column. It lists the values that can be used.

I think we can call this one closed now.