joequery/Stupid-Table-Plugin

What happened to data-sort-desc?

Closed this issue · 6 comments

Hey Joe, haven't checked in with this project in a little while but I was just setting up a new table and found that the data-sort-desc attribute isn't working.

Looking in the code I don't see any checks for .data('sort-desc') or similar. Was it removed?

Went back through the history and it looks like it's commit 4dbd295 that introduced this bug. Unless there is an alternate way to create separate asc/dec sorting functions I assume you do still want this functionality? I'll add it back in if so.

You're right, I didn't realize that feature was removed.

However, I think some sort of hook that allows you to manually adjust the column array after the sort but before the rendering would allow people to accomplish the things we used data-sort-desc for. The hook could have access to the data sort direction if the logic really relied upon the direction.

Yes that could work, although it seems a little inefficient to me as we end up sorting the list twice (the plugin sorts it, then I have to re-sort it). Perhaps a hook before sorting would work? That way you could change the column's values before sorting.

In my particular case I want to keep blank values at the bottom, so we have [1, 2, 3, --] when sorting up and [3, 2, 1, --] when sorting down. With a beforedatasort* callback I can change the blanks to either 0 or 99 depending on the sort direction.

* feel free to suggest a better name :)


Edit: after trying a few things I don't think it will work. When you are passed the column of data to sort, the only way of knowing if you need to run the custom sorting is by explicitly checking the column number.

To me it still feels like this functionality should be tied to a custom sorting function somehow.

I was writing up this huge comment about my personal views of data integrity and whatnot, then I realized although there is the occasionally need for ascending/descending sorts to provide different results, I personally just feel like the sorting function itself isn't the place to accomplish this.

I have to compromise on my strict adherence to data correctness and predictability since it's not so uncommon to have to sort data that's not entirely in the correct form. For example, a None or "" displaying in an integer column. Unfortunately you need to give non-valid values some arbitrary valid value just for the sorting operation to not invoke undefined behavior. Since

parseInt(5, 10) - parseInt("", 10) => NaN

That means we have to assign some numeric value to "" just so the whole integer sort doesn't fall apart. But the nice thing about using a alterColumnRows callback (name subject to change :P) is that

  1. The value we assign to the invalid data doesn't need a semantic meaning
  2. Since the value has no semantic meaning, we don't have to care about the other values in the column.

The example beforedatasort idea requires you to know the upper and lower bounds of your data set. Sometimes that's not possible, and sometimes the data set is constantly changing.

The alterColumnRows callback would let us safely remove invalid values from the column array and insert them at the end or beginning, depending on our intentions. Maybe we want blanks to display at the beginning when sorting ascending but at the end when sorting descending. Maybe we want blanks to always display at the end. The nice thing about moving invalid data to the beginning/end of a sorted array is that the valid data is still sorted, regardless of whatever value was assigned to the invalid data.

Suppose we're sorting the integer column [5, "", -3, 1, ""]. If we assign data-sort-value of empty strings to 0, we get the following when we sort the column [-3, "", "", 1, 5]. If we move the blank values to the end of the column, we get [-3, 1, 5, "", ""], and the valid data is still sorted. This operation works for any arbitrary valid data-sort-value assigned to empty strings. Or you can even have a custom sort function that checks for empty string and assigns it to 0.

Using a alterColumnRows callback will be slightly more inefficient than specifying ascending/descending sorts, but most likely it won't be inefficient enough to have any noticeable performance hits for two reasons.

  1. I imagine the bottleneck in this entire plugin is dom insertion, not the sorting operations themselves (though a test to verify this would be a nice-to-have. We could test on the large table example you made). I hypothesize that inefficiencies introduced by alterColumnRows will be negligible compared to the time required to repaint the table rows, which is something we have no control over.
  2. As long as the specified alterColumnRows function is within O(n) time, the complexity of doing the sort and then the manipulations will be equal to the complexity of just doing the sort. This is because O(n log n) + O(n) is considered to be O(n log n), and O(n log n) is the time complexity for merge/quicksort, which I imagine the JS sort() implementations are using across all browsers. Even then, will an O(n^2) alterColumnRows callback outweight the dom repainting? I personally think not, but that needs verification.

Having the beforedatasort callback changing specific values based on sorting direction would require you to iterate over the array to find the values you'd like to change. The operation would be O(n).

We can be a bit clever and say "if the column is already sorted, we know the blanks are at the end, so break the loop that sets the sort value once we're no longer looking at a blank", but that's still considered O(n), and I imagine a minor optimization not worth making.

On balance, the "alterColumnRows" system seems like a better way to go, and not really inefficient.

I think the column issue I mentioned is not a big one. (In fact it already exists for the current callbacks.) The only thing I'd suggest is maybe it would be valuable to pass the type of column being sorted into the callback, since -- may mean different things in an int or string column, i.e. I can easily make all my int columns check for blanks without having to store/maintain the exact column indices.

So for alterColumnRows callback we'd have

  • column (the column index)
  • direction (asc/desc)
  • datatype (int/string etc)
  • data or columnRows (the array of values)

If you're happy I can go ahead and work on this in the next few days.

Finally got around to looking at this. I've pushed a branch "altercolumnrows" with the change. I decided to call the event afterdatasort as that made more sense to me as an actual event name.

This approach seems pretty flexible for whatever way people want to modify data. However, it took me longer than expected to get the example callback right (see complex.html). Maybe my JS is just a little rusty? (I only mention this because I worry that some users might struggle trying to utilise the callback...)

Anyway, take a look at 88575e0 and let me know what you think.