Sort doesn't work out of the box with dates #83

Closed
alanrubin opened this Issue Sep 6, 2012 · 3 comments

4 participants

@alanrubin

Hi,

setSort method for clientPager doesn't work for date object right now, as it invokes .toString() method for converting the object for comparison, as implemented in the code:

_sort: function ( models, sort, direction ) {
            models = models.sort(function (a, b) {
                var ac = a.get(sort),
                    bc = b.get(sort);

                if ( !ac || !bc ) {
                    return 0;
                } else {
                    /* Make sure that both ac and bc are lowercase strings.
                    * .toString() first so we don't have to worry if ac or bc
                    * have other String-only methods.
                    */
                    ac = ac.toString().toLowerCase();
                    bc = bc.toString().toLowerCase();
                }

I know I can implement toString() method for dates, but I think it is dangerous to override an existing method of a native object and force us to use string for comparison.

It could be better to have it support all native js objects out of the box and in case the data being compared is a "object" also standardize a method such as .compareValue() so that developers could easily extend their custom objects and support sorting. If you think it is relevant, I will create a pull request for that.

Cheers,
Alan

@ingro

I think that would be very nice!

@addyosmani
Backbone Paginator member

@alexandernst I agree regarding overriding existing methods of native objects. Maybe we should consider supporting all natives out of the box as suggested?

@alexandernst
Backbone Paginator member

@addyosmani Indeed, while I was writing that I didn't think about dates. @alanrubin Patches are welcome, or you can wait me to write them (but I'll be a little bit busy for a few weeks)

@addyosmani addyosmani closed this May 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment