Skip to content
This repository has been archived by the owner on May 17, 2018. It is now read-only.

Comparator definition redundancy #89

Closed
gbedardsice opened this issue Jun 5, 2013 · 5 comments
Closed

Comparator definition redundancy #89

gbedardsice opened this issue Jun 5, 2013 · 5 comments

Comments

@gbedardsice
Copy link
Contributor

When using Backbone.Pageable in client mode with Backgrid, I found that if I wanted to replace the default comparator function, I had to override it at two places. Here and here in Backgrid.

This seems odd to me. Is there any reason setSorting could not take a comparator or even a makeComparator option?

@wyuenho
Copy link
Member

wyuenho commented Jun 5, 2013

You generally don't need to override _makeComparator. setSorting is a convenient function that will create a comparator and set it to the collection for you.

I'm curious tho, what problem is the default behavior causing you that you need to override the default comparator?

@gbedardsice
Copy link
Contributor Author

I'm using a natural sort comparator. I know I should probably rely on server side paging for this type of operation, but right now it's not possible for me.

@wyuenho
Copy link
Member

wyuenho commented Jun 5, 2013

Sometime I wish JS had operator overloading :)

I didn't anticipate this use case and I just assumed whatever you are sorting on will have a natural order comparable by the language's default comparison operators.

Another reason is that in the case the collection is not a pageable collection, a comparator will need to be generated and attached to the collection.

Lastly, the reason setSorting doesn't take a comparator is because besides setting a comparator to the collection, I also need to know the order in order to change state.order. I wouldn't know that just by looking at the comparator.

@wyuenho
Copy link
Member

wyuenho commented Jun 5, 2013

So I think if you are using backgrid and pageable, and need to supply your own comparator, you pretty much have to override it in 2 places.

@wyuenho wyuenho closed this as completed Jun 5, 2013
@gbedardsice
Copy link
Contributor Author

Yeah, it is tricky. Thanks for answering.

gbedardsice pushed a commit to gbedardsice/backbone-pageable that referenced this issue Jun 7, 2013
@wyuenho wyuenho reopened this Jun 7, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants