Skip to content
This repository has been archived by the owner on Apr 2, 2019. It is now read-only.

custom comparator #219

Closed
larryhengl opened this issue Jun 6, 2013 · 25 comments · Fixed by #226
Closed

custom comparator #219

larryhengl opened this issue Jun 6, 2013 · 25 comments · Fixed by #226

Comments

@larryhengl
Copy link

Question: what would be the canonical approach to implementing a custom sort comparator for PageableCollections in Client mode? Looking to use an ignoreCase sorting fn for strings. Tried overriding the comparator in the collection def, but it seems to fallback to native or _cid sorting. Tried to extend the HeaderCell view and pass in a new comparator def there, but not yet successful. What would be your approach? Many thanks!

@carlygeehr
Copy link

Each cell in my grid can be a string, JSON object, or array, so I definitely needed a custom comparator - I ran into similar issues and ended up just having to modify the _makeComparator function in backbone-pageable.js. Probably not the "right" way to do it, but it worked for me...

I'd love to hear solutions from others who've done a custom comparator.

@wyuenho
Copy link
Contributor

wyuenho commented Jun 7, 2013

@gbedardsice has asked a similar question recently: backbone-paginator/backbone-pageable#89

This seems to be a recurring issue for a couple of people of late. Suggestions on how to improve this is welcomed.

@thomasfl
Copy link

thomasfl commented Jun 7, 2013

This may be a bit off topic, but I simply want case insensitive column sorting.

I would prefer to have "comparator : function(a,b){...}" as a column option, but any hints on how to do this without modifying backgrid would be appreciated.

@gbedardsice
Copy link
Contributor

The problem I see with passing a custom comparator function is that it depends on the column. It would have to be a dumbed down comparator that doesn't consider left and right as models, but as attributes.

@thomasfl
Copy link

thomasfl commented Jun 7, 2013

What about passing sorting: 'caseinsensitive' | 'numeric' | 'date' instead? It's not as flexible, but it would save my day. ;-)

@wyuenho
Copy link
Contributor

wyuenho commented Jun 7, 2013

How about this?

columns: [
  name: "name",
  label: "Name",

  // Put your custom logic that returns a comparator function based on the direction here
  // Similar to Backbone.PagableCollection#_makeComparator
  comparatorProvider: function (attr, order) {
    return function (left, right) {

      // no-op
      if (order == null) return 0;

      var l = left.get(attr), r = right.get(attr), t;

      // if descending order, swap left and right
      if (order === 1) t = l, l = r, r = t;

      // compare as usual
      if (l === r) return 0;
      else if (l < r) return -1;
      return 1;
    }
  }
]

This should help folks who are dealing with nested data who only have Formatters to use to display them but not sort them.

@gbedardsice
Copy link
Contributor

This could work, but why would this logic (the swapping of left and right) ever change? Why not have it built in inside HeaderCell?. Let me build up a commit real fast, I'll show you what I mean.

@wyuenho
Copy link
Contributor

wyuenho commented Jun 7, 2013

Are you thinking of a function that lets you extract the values from the models?

gbedardsice pushed a commit to gbedardsice/backgrid that referenced this issue Jun 7, 2013
@gbedardsice
Copy link
Contributor

What do you think of something like that? Keep in mind this is not tested at all, just a quick mockup.

@gbedardsice
Copy link
Contributor

The only ambiguity I see with this is that the comparator option you would pass to the HeaderCell is not a typical Backbone.Collection comparator. it's a generic version that compares the two raw values instead of the two models.

@gbedardsice
Copy link
Contributor

Also see this commit: backbone-paginator/backbone-pageable@02a0819

@wyuenho
Copy link
Contributor

wyuenho commented Jun 7, 2013

I'm thinking along similar lines. I think most people will only have trouble with this line:

var l = left.get(attr), r = right.get(attr), t;

That's pretty much the only line they'll have to customize, the rest should be the same for everyone. How about just a value extractor that you can provide to the column definition?

BTW, that commit you just posted looks like it's coming from backbone-pageable's repo, how did you do that?

@gbedardsice
Copy link
Contributor

How about just a value extractor that you can provide to the column definition?

Are you talking about something like this?

var l = left.get(this.column.modelAttribute()), r = right.get(this.column.modelAttribute())

BTW, that commit you just posted looks like it's coming from backbone-pageable's repo, how did you do that?

The same way you linked to the backbone-pageable issue, except you replace the issue number (#89) with @ followed by the commit hash (@02a0819) !

@thomasfl
Copy link

thomasfl commented Jun 8, 2013

Why is it a problem to supply a simple, classic comparator function?

function compare(a, b) {
    if (a.toString() < b.toString()) return -1;
    if (a.toString() > b.toString()) return 1;
    return 0;
}

@wyuenho
Copy link
Contributor

wyuenho commented Jun 8, 2013

@gbedardsice Something like this:

columns: [
  name: "name",
  label: "Name",
  value: function (model, attr) {
    return model.get(attr).b.c[3];
  }
]

The value will be used as the left and right values for the comparator generated automagically by HeaderCell.

If not supplied, the value function will default to:

function (model, attr) {
 return model.get(attr);
}

@gbedardsice
Copy link
Contributor

So everywhere that model.get(column.get("name")) is used will be changed to this? If so, what about the other way around, will you have a value injector?

@wyuenho
Copy link
Contributor

wyuenho commented Jun 8, 2013

No. Only when extracting the attribute value for sorting. You already have formatters that you can override to process nested data.

@larryhengl
Copy link
Author

@wyuenho I'm starting to like your idea of defining the sort value/attr in the column/cell def. I was trying to pass that into the comparator factory.

I took the approach of extending the HeaderCell with a custom comparator, in my case was for Ignore Case + Nulls Last. So it started looking like...

var IgnoreCaseHeaderCell = Backgrid.HeaderCell.extend({
  sort: function(columnName, direction){
    return Backgrid.HeaderCell.prototype.sort.call(this, columnName, direction, ignoreCaseSortMaker(columnName, direction));
  }
}); 

and the ignoreCase factory started looking like:

var ignoreCaseSortMaker = function(attr, direction) {
  return function (a, b) {
    var left  = a.get(attr);
    var right = b.get(attr);
    var order;

    if (direction === "ascending") order = -1;
    else if (direction === "descending") order = 1;
    else order = null;

    // no-op
    //if (order === null) return 0;

    if (_.isUndefined(left)) { return 1; }
    else if (_.isUndefined(right)) { return 1; }

    var l = left[0], r = right[0], t;

    // if descending order, swap left and right
    if (order === 1) t = l, l = r, r = t;

    if (l === r) return 0;

    if (_.isString(l)) {
      l = l.toLowerCase();
      r = r.toLowerCase();
    }

    return l < r ? -1 : 1;
  };
};

i'm not sure the logic is working correctly here. but the issue i see is the sort doesn't kick in until the 3rd header sort click, or when direction is removed...and then it blows up. And the 1st and 2nd header sort clicks are hitting the default comparators instead. With sort methods being defined in multiple places (header and pageable extensions) it's challenging to hit on a single fix for this.

and of course i set the respective header in the col def depending on cell type. sort of like:

...
{"name": blah, 
 "label": blah, 
 "cell": blah_type, 
 "headerCell": (blah_type === "string" ? IgnoreCaseHeaderCell : Backgrid.HeaderCell), 
 "editable": blah || false
}, ...

@gbedardsice
Copy link
Contributor

@wyuenho Ok I see what you mean. Makes sense. Also, concerning the Backbone.Pageable commit, it's because the commit is directly on the Backbone.Pageable repo, not on Backgrid's version of the lib!

@wyuenho
Copy link
Contributor

wyuenho commented Jun 10, 2013

I know I know I have a fix for this and the sortable: "somestring" in a few days. Feel free to send over a PR if you are impatient.

gbedardsice pushed a commit to gbedardsice/backgrid that referenced this issue Jun 10, 2013
gbedardsice pushed a commit to gbedardsice/backgrid that referenced this issue Jun 10, 2013
@thomasfl
Copy link

Yes, this seems perfect.

var comparator = function(left, right) {
      var l = left.toLowerCase(), r = right.toLowerCase();
      if(l === r) return 0;
      if(l < r) return -1;
      return 1;
    };

    cell = new Backgrid.HeaderCell({
      collection: words,
      column: {
        name: "name",
        cell: "string",
        comparator: comparator
      }
    });

@wyuenho wyuenho reopened this Jun 12, 2013
@wyuenho
Copy link
Contributor

wyuenho commented Jun 12, 2013

Reopening until I'm done moving things around...

wyuenho added a commit that referenced this issue Jun 15, 2013
@wyuenho wyuenho closed this as completed Jun 15, 2013
@mparisi76
Copy link

How can we find out what library or libraries need to be updated in order to utilize the new sorting changes? I updated just backbone-pageable.js and now the paginator links are broken. When I click on a page other than page 1, it moves to the page, but the 1 remains highlighted. I'm not sure if there's something else I need to do/update or if it's a bug..

@wyuenho
Copy link
Contributor

wyuenho commented Jun 19, 2013

@mparisi76 According to #231 this issue is fixed if you update to the latest paginator extension from master. Ping me on #231 if it doesn't work out for you.

I'll release 0.3 this week. Will document an upgrade path.

@v8187
Copy link

v8187 commented Nov 11, 2014

@wyuenho I just started using Backgrid, and need to exclude group-headings while sorting. Please help.

Regards,
Vikram

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants