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 a way to override the default sort #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pmq
Copy link
Contributor

@pmq pmq commented Nov 16, 2014

Hi Jose,

First of all, thanks for ngReactGrid, it works well. We needed a way to be able to specify our custom sort on some columns, hence this first draft. Before we go further on this (the current PR is crude), just wanted to know your opinion on this feature. Would there be enough interest to warrant merging at some point?

Thanks.

@josebalius
Copy link
Owner

@pmq sounds interesting, definitely interested in merging this functionality.

@josebalius
Copy link
Owner

@pmq is this complete? or still waiting to submit more?

@pmq
Copy link
Contributor Author

pmq commented Nov 18, 2014

@josebalius I'm waiting to push this into production for us, and see if the users complain :) It should only be a few days.

Two questions though, to adapt the PR :

  1. right now I've put the sort function in sortInfo, maybe this should go top-level, just like ngGrid used to do with sortFn?
  2. in your original object comparison code, you have two possible return values (1 and -1), but not 0 to denote equality; should I do the same (right now I return 0 in case the two objects are equal)?

@pmq
Copy link
Contributor Author

pmq commented Nov 30, 2014

@josebalius I didn't forget this PR - right now we're running this and it seems OK. The two questions still stand, but if you want to review as-is, it's also fine for me. Let me know!

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

Successfully merging this pull request may close these issues.

2 participants