Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add column sorting #7

Closed
lsmith opened this issue Dec 13, 2011 · 8 comments
Closed

Add column sorting #7

lsmith opened this issue Dec 13, 2011 · 8 comments

Comments

@lsmith
Copy link
Owner

lsmith commented Dec 13, 2011

Class extension that auto-mix()es onto Y.DataTable

Should provide a method sort(keyOrColIndex, descending?) or sort(keyOrIndex[, keyOrIndex]*) on the DataTable proto.
Should provide a sortable attribute that accepts:

  • 'auto' (default) - reads columns config for columns defined with sortable: true
  • true - all columns are sortable
  • false - none are sortable
  • array of keys/indexes - specified columns are sortable

Since sorting can be considered a UI-specific operation[1], the implementation might leverage ModelList's sorting capability, but do so by assigning the data Models to an alternate ML implementation that had the sorting in place for the bodyView instance. The view ML should be synced to the DT ML via events, but there should be almost no memory overhead, since no new Models would be created, although there would be some risk that the DT's ML included an additional API that a custom bodyView expected to be in place.

An additional challenge is how a DT class extension would apply the sort arrow UI to the header cells. One idea would be to do this with a Plugin which was applied to the headerView instance. Since header rendering is not a performance vector, another option would be to have the class extension apply an after('render') to add the sort arrows. On the face of it, I prefer the Plugin idea, because there's room to configure the specific sort plugin with an additional DT attribute, contributed by teh class extension.

[1] Technically speaking, order is state, but a UI sort doesn't necessarily represent a data state change.

@lsmith
Copy link
Owner Author

lsmith commented Dec 14, 2011

The proposal for sort(keyOrIndex[,keyOrIndex...]) would support descending sort by prefacing the key or column number with -. e.g.

// sort by lastName ascending, then firstName descending, and dob descending
table.sort('lastName', '-firstName', '-dob');

I'm not a big fan of signatures with n args, but it does seem like a simple way to support multi-column sort and direction. Feedback welcome.

@lsmith
Copy link
Owner Author

lsmith commented Dec 14, 2011

The yuilibrary.com ticket tracking this issue is http://yuilibrary.com/projects/yui3/ticket/2531130

@lsmith
Copy link
Owner Author

lsmith commented Dec 14, 2011

From a lengthy conversation in #yui, it was agreed that sorting should be applied to the DT's ML directly. If an implementer wants a data set that maintains a specific order, they should maintain that data in a separate ML and just share its Models in the DT's ML.

@lsmith
Copy link
Owner Author

lsmith commented Dec 15, 2011

More feedback from #yui discussions:

table.sort('name');
table.sort(['name', 'dob']);
table.sort({ name: 'asc' }); // or sort({ name: 1 })
table.sort({ name: 'desc' }); // or sort({ name: -1 })
table.sort(['name', { dob: -1 }]);
table.sort('name', { sort event payload object });

table.toggleSort(); // reverse the direction of all sorted columns
table.toggleSort('name'); // toggles dir for name column, leaves dob column sort as it was
table.toggleSort(['name', 'dob']); // toggleSort({ name: dir}) and toggleSort([{ name: dir },...]) are NOT supported
table.toggleSort('name', { sort event payload object });

@lsmith
Copy link
Owner Author

lsmith commented Jan 19, 2012

There's an awkward issue when trying to make a column sortable whose content is generated by a formatter.

The options, it seems to me are:

  1. require all sortable columns to have an associated key,
  2. require keyless columns to supply a sortFn
  3. (unspeakable) use the rendered content as a value somehow to sort

(1) is the current requirement, but is undesirable because sorting would lose fidelity from rendered content, though sortFn would still be supported
(2) is my favorite, but runs into an issue with the ModelList comparator (see below)
(3) would be horribly slow if the cell content had to be fetched inside the sort comparator, or memory consumptive and awkward architecturally (and need lots of extra code) to store the cell content either in the model, in the DT instance, or on the headerView or bodyView. There's no way I'm doing this without a big fight.

Another issue is that the ModelList's sorting algo doesn't support multi-key/complex sorting. The comparator function outputs a value that is then compared with simple < and >. This would work fine if sort order was derived by the value in one attribute, but if subsorting is needed, then the comparator can't intelligently output a value generated from multiple attribute that will result in reliable sorting (e.g. with a number + name based sort, a comparator might return "605Jo" for one Model vs "67Alice" for another.)

Another result of the comparator logic is that columns with custom sortFns would have no way to apply that function, which prevents (2), and sortFn needs to be there for backward compatibility (sort of, that part is broken).

The plan for the sortable extension was to set the comparator on the ModelList, but it's looking like I need to override _sort and _findIndex as well, which doesn't feel right.

@rgrove
Copy link

rgrove commented Jan 19, 2012

I think we can solve the sorting issues fairly easily by making a couple of changes to ModelList:

  1. Add a _compare() method that accepts two arguments and returns a negative value if the first should be sorted before the second, 0 if they're equivalent, positive if the first should be sorted after the second. This is essentially what _sort() was already doing, but the values are assumed to have already been returned from the comparator function, so _compare() won't call the comparator again.
  2. _findIndex() and _sort() will call _compare() to determine how to order any two items relative to one another.

This makes _compare() an easy override point for custom sorting logic. For example, you could override it to flip the order (for a descending sort) or to make it multi-column aware (the comparator could return an array of column values, which _compare() could then use as sort criteria). As long as you implement your comparison logic in _compare(), then both _sort() and _findIndex() will use it automatically and you won't need to do any extra work.

How does this sound?

@lsmith
Copy link
Owner Author

lsmith commented Jan 19, 2012

LGTM

@lsmith
Copy link
Owner Author

lsmith commented Jan 27, 2012

Pushed to master

@lsmith lsmith closed this as completed Jan 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants