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

Sorting with nulls #940

Closed
wants to merge 2 commits into from
Closed

Sorting with nulls #940

wants to merge 2 commits into from

Conversation

dustinsgoodman
Copy link

closes #939

@c0bra
Copy link
Contributor

c0bra commented Jan 23, 2014

Dustin,

Pull requests need to include changes to the source files, not to the
compiled build files. Also unit tests are always welcome!

Thanks for the contribution!
On Jan 22, 2014 7:16 PM, "Dustin Goodman" notifications@github.com wrote:

closes #939 #939

You can merge this Pull Request by running

git pull https://github.com/sharocko/ng-grid sorting-with-nulls

Or view, comment on, or merge it at:

#940
Commit Summary

  • prevents nulls from creeping back to the top of sort result
  • allow fully custom sorts

File Changes

Patch Links:


Reply to this email directly or view it on GitHubhttps://github.com//pull/940
.

@dustinsgoodman
Copy link
Author

Oops! Not fully accustomed to the project hierarchy. I'll fix this in the morning and add tests. :)

- keep nulls at bottom of list regardless of sort order
- if user provides custom sort, allow them to have full control over sort
@dustinsgoodman
Copy link
Author

@c0bra Okay, I fixed my commits to actually only affect the source instead of the builds. I went in to write the tests and found that there weren't any predefined tests for the sortService's sortData function. I started to write them but kept hitting different roadblocks because of the set up required to get to the sorting. I don't have time this week to write the tests but I have manually tested this fix and it is now running on my production system. I'll leave it up to you guys what you do with it, but I definitely think this a fix worth implementing in 2.0.8 or 3.

@c0bra c0bra added this to the 2.0.8 milestone Mar 21, 2014
@c0bra
Copy link
Contributor

c0bra commented Mar 21, 2014

Here's a place to discuss sorting in 3.0: #1072

@dustinsgoodman
Copy link
Author

@c0bra I'm going to put a comment in the new ticket regarding this issue. I think if the issue is addressed in 3.0 then this ticket can be closed.

@c0bra
Copy link
Contributor

c0bra commented Apr 22, 2014

@dustinsgoodman I went ahead and merged this into 2.0.8; looked good to me! 6a6143c

@c0bra c0bra closed this Apr 22, 2014
@dustinsgoodman
Copy link
Author

@c0bra Thanks man! Hopefully the fix gets carried into 3.0! :)

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.

None yet

2 participants