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

allow replacing null keys with specified value in groupBy filter #41

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

Conversation

AlexCuse
Copy link

I gave up on trying to figure out why, but the combination of groupBy, toArray and angular's built-in orderBy filter had some disastrous side effects for me. So added an option to replace null/undefined keys as a means to control where missing keys fall in the natural ordering.

It seems to build OK and the tests pass, I just didn't check the combined files in because a "reverseArray" function that I didn't touch is showing as removed in angular-filter.js.

@a8m
Copy link
Owner

a8m commented Oct 18, 2014

@AlexCuse Thx for the PR.
Could you provide a Plunker example (plnkr.co) ?
Why not use the defaults filter for undefined values ?
e.g:

$scope.players = [
    { name: 'Gene',   team: 'alpha' },
    { name: 'George', team: 'beta'  },
    { name: 'Steve',  team: 'gamma' },
    { name: 'Paula',  team: 'beta'  },
    { name: 'Scruath',team: 'gamma' },
    { name: 'Dan'                   },
    { name: 'Alex'                  }
 ];
$scope.fallback = { team: 'others' }; 
<ul ng-repeat="(key, value) in players | defaults:fallback | groupBy: 'team'" >
  Group name: {{ key }}
  <li ng-repeat="player in value">
    player: {{ player.name }}
  </li>
</ul>

let me know what do you think...

@AlexCuse
Copy link
Author

I didn't see the defaults filter, does seem like that would work.

Here is a plunker using defaults: http://plnkr.co/edit/rEDbqLkQ4ZaZ9z2Dlmro?p=info

And another using the null replacement syntax I added: http://plnkr.co/edit/RYc3RriigIJYgHF9vGlJ

As long as the defaults can be inlined like that (so I can tell why I am doing something special with the default value later on) I think defaults would be fine, if a bit less concise.

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

Successfully merging this pull request may close these issues.

2 participants