Skip to content

Commit

Permalink
fix: Allow setting a ngClass on <ui-select> element
Browse files Browse the repository at this point in the history
Since a ngClass is already present in the uiSelect theme templates, we could not add any ngClass on <ui-select> without getting an JS error
Currently, adding a ngClass on <ui-select> result in:
`ng-class="{class: expression} {open: $select.open}"`
With this fix, doing the same thing result in:
`ng-class="{class: expression, open: $select.open}"`

Closes angular-ui#277
  • Loading branch information
Jean-Baptiste Lenglet committed Feb 17, 2016
1 parent b1c4fdc commit 6a99b08
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions src/uiSelectDirective.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ uis.directive('uiSelect',
controllerAs: '$select',
compile: function(tElement, tAttrs) {

// Allow setting ngClass on uiSelect
var match = /{(.*)}\s*{(.*)}/.exec(tAttrs.ngClass);

This comment has been minimized.

Copy link
@aaronroberson

aaronroberson Feb 18, 2016

Using .exec is not ideal. Can this fix be achieved with directive priority instead?

This comment has been minimized.

Copy link
@Magador

Magador Feb 18, 2016

Owner

Does not seem so, according to angular/angular.js#5695 .
Else, we have to use class="{condition ? 'classname': ''}"

This comment has been minimized.

Copy link
@aaronroberson

aaronroberson Feb 18, 2016

How about using compile, as in this example: http://jsfiddle.net/3vMyj/2/

This comment has been minimized.

Copy link
@Magador

Magador Feb 18, 2016

Owner

I did a small jsperf: http://jsperf.com/exec-vs-replace
It seems that .exec is faster than .replace

if(match) {
var combined = '{'+ match[1] +', '+ match[2] +'}';
tAttrs.ngClass = combined;
tElement.attr('ng-class', combined);
}

//Multiple or Single depending if multiple attribute presence
if (angular.isDefined(tAttrs.multiple))
tElement.append('<ui-select-multiple/>').removeAttr('multiple');
Expand Down

2 comments on commit 6a99b08

@aaronroberson
Copy link

Choose a reason for hiding this comment

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

Right on, I've merged it in for the next release!

@Magador
Copy link
Owner

Choose a reason for hiding this comment

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

👍

Please sign in to comment.