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

Add choices grouping using 'group by' expression #49

Merged
merged 2 commits into from
Jul 11, 2014
Merged

Add choices grouping using 'group by' expression #49

merged 2 commits into from
Jul 11, 2014

Conversation

just-boris
Copy link
Contributor

As described in #48, I've done options grouping.
Here is plunker where you can see this changes in action.

@filipedeschamps
Copy link

Please merge this feature.

1 similar comment
@MarcosRava
Copy link

Please merge this feature.

@FabricioFFC
Copy link

@tkrotoff please merge this feature, it's very helpful

@sheerun
Copy link
Contributor

sheerun commented May 18, 2014

+1

2 similar comments
@kasperisager
Copy link

👍

@leipert
Copy link

leipert commented Jun 3, 2014

👍

@garrettheel
Copy link

The only issue I have with this is that the value of group by has to be an attribute name rather than an expression which is inconsistent with angular core. It also prevents doing anything more complex like generating an option group label dynamically.

Other than that it looks great, +1 from me

@dimirc
Copy link
Contributor

dimirc commented Jun 29, 2014

@tkrotoff @ProLoser what is your opinion on this feature?

@just-boris
Copy link
Contributor Author

Just have rebased code from the current master. I have to implicitly declare angular version, because later versions are incompatible and fail the tests

@dimirc
Copy link
Contributor

dimirc commented Jul 1, 2014

@just-boris thanks for pointing out that declaring angular version fixes the failing tests. That actually makes sense since after 1.2.18 we have some changes on how angular manages ngTransclude and that affects the directive. PR #91 is already in progress to fix 1.2.18+

@dimirc
Copy link
Contributor

dimirc commented Jul 1, 2014

@just-boris I created and merged #95 to fix this asap

@just-boris
Copy link
Contributor Author

rebased again.
Please make a decision about this PR, it is so difficult to keep my changes in sync.

@davidmdem
Copy link

👍

@ProLoser
Copy link
Member

ProLoser commented Jul 1, 2014

@dimirc @tkrotoff please feel free to do what you guys think is best as I'm no longer actively working on these projects. My one concern I will have is feature and code bloat. If there is absolutely no other way to achieve this other than creating an explicit syntax for it then so be it. But I wonder about alternative edge-cases (such as nested arrays for grouping instead of grouping by a property). I'd much rather provide users with a strong api like an iterator callback or something to allow users to do this themselves. For instance, if the dev turns this into a nested array or object, couldn't he just do a double ng-repeat inside his custom template or something?

Anyway, you guys should do what you wish, and if you find some people are very active on the project feel free to add them to the org.

@filipedeschamps
Copy link

A little detail about this implementation is, if there's no group (even if
you don't declare a group by expression), it will create a blank element at
the start of the list.

On Tue, Jul 1, 2014 at 6:52 PM, Dean Sofer [email protected] wrote:

@dimirc https://github.com/dimirc @tkrotoff
https://github.com/tkrotoff please feel free to do what you guys think
is best as I'm no longer actively working on these projects. My one concern
I will have is feature and code bloat. If there is absolutely no other way
to achieve this other than creating an explicit syntax for it then so be
it. But I wonder about alternative edge-cases (such as nested arrays for
grouping instead of grouping by a property). I'd much rather provide users
with a strong api like an iterator callback or something to allow users to
do this themselves. For instance, if the dev turns this into a nested array
or object, couldn't he just do a double ng-repeat inside his custom
template or something?

Anyway, you guys should do what you wish, and if you find some people are
very active on the project feel free to add them to the org.


Reply to this email directly or view it on GitHub
#49 (comment).

@dimirc
Copy link
Contributor

dimirc commented Jul 8, 2014

Instead of the explicit syntax, I think that we could better have an attribute and pass a string (the property that we want to use for grouping) or function for a more complex operation. Similar to _.groupBy

<choices group-by="'age'" repeat="item in people | filter: $select.search">
  <span ng-bind-html="item.name | highlight: $select.search"></span>
  <small ng-bind-html="item.email | highlight: $select.search"></small>
</choices>

or

<choices group-by="someFunction" repeat="item in people | filter: $select.search">
  <span ng-bind-html="item.name | highlight: $select.search"></span>
  <small ng-bind-html="item.email | highlight: $select.search"></small>
</choices>

@just-boris
Copy link
Contributor Author

@dimirc ok, it will be easier to realize than have an additional token in repeat attribute, so I agree with you.
Should I change the API, and then will my changes be merged?

@dimirc
Copy link
Contributor

dimirc commented Jul 8, 2014

Yes, will be perfect if you could do the changes and we'll merge this feature asap

Sent from my iPhone

On Jul 8, 2014, at 2:08 AM, Boris Serdyuk [email protected] wrote:

@dimirc ok, it will be easier to realize than have an additional token in repeat attribute, so I agree with you.
Should I change the API, and then will my changes be merged?


Reply to this email directly or view it on GitHub.

@just-boris
Copy link
Contributor Author

@dimirc, done

ctrl.groups[groupValue].push(item);
}
});
angular.forEach(ctrl.groups, function(group, key) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of clearing the arrays for each group and then deleting empty one, why not just always starting with a new empty object for ctrl.groups?

Like this

      function updateGroups(items) {
        ctrl.groups = {};
        angular.forEach(items, function(item) {
          var groupFn = $scope.$eval(groupByExp);
          var groupValue = angular.isFunction(groupFn) ? groupFn(item) : item[groupFn];
          if(!ctrl.groups[groupValue]) {
            ctrl.groups[groupValue] = [item];
          }
          else {
            ctrl.groups[groupValue].push(item);
          }
        });
        setPlainItems(items);
      }

@just-boris
Copy link
Contributor Author

I can reproduce a bug with key navigation in just one case: when grouped by value with both lower and uppercase letters: http://plnkr.co/edit/pvO8aL?p=preview
I've fixed it and improved a test to cover it
@dimirc does this fix help you? Now I create array of plain items regarding by its groups are sorted in list and all working correctly.

@dimirc dimirc mentioned this pull request Jul 10, 2014
dimirc added a commit that referenced this pull request Jul 11, 2014
Add choices grouping using 'group by' expression
@dimirc dimirc merged commit 4d8ea26 into angular-ui:master Jul 11, 2014
@dimirc
Copy link
Contributor

dimirc commented Jul 11, 2014

@just-boris some additional changes will be merged at #102, thanks for your PR and any additional help to improve the library are welcome

@dimirc
Copy link
Contributor

dimirc commented Aug 11, 2014

@just-boris if you have some free time, will be nice to have some help on PR #138, specially on the task "Check choices with groupby to work correctly on "multiple" mode"

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

Successfully merging this pull request may close these issues.