Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Commit 7c80d8a

Browse files
gkalpakgkalpak
committed
refactor(ngClass): remove redundant $observer and dependency on $animate
Includes the following commits (see #15246 for details): - **refactor(ngClass): remove unnecessary dependency on `$animate`** - **refactor(ngClass): remove redundant `$observe`r** The code was added in b41fe9f in order to support having both `ngClass` and interpolation in `class` work together. `ngClass` has changed considerably since b41fe9f and for simple cases to work the `$observe`r is no longer necessary (as indicated by the expanded test still passing). That said, it is a [documented known issue][1] that `ngClass` should not be used together with interpolation in `class` and more complicated cases do not work anyway. [1]: https://docs.angularjs.org/api/ng/directive/ngClass#known-issues
1 parent d05f894 commit 7c80d8a

File tree

2 files changed

+103
-57
lines changed

2 files changed

+103
-57
lines changed

‎src/ng/directive/ngClass.js

+5-12
Original file line numberDiff line numberDiff line change
@@ -8,19 +8,15 @@
88

99
function classDirective(name, selector) {
1010
name = 'ngClass' + name;
11-
return ['$animate', function($animate) {
11+
12+
return [function() {
1213
return {
1314
restrict: 'AC',
1415
link: function(scope, element, attr) {
1516
var oldVal;
1617

1718
scope.$watch(attr[name], ngClassWatchAction, true);
1819

19-
attr.$observe('class', function(value) {
20-
ngClassWatchAction(scope.$eval(attr[name]));
21-
});
22-
23-
2420
if (name !== 'ngClass') {
2521
scope.$watch('$index', function($index, old$index) {
2622
/* eslint-disable no-bitwise */
@@ -69,12 +65,9 @@ function classDirective(name, selector) {
6965
var toRemove = arrayDifference(oldClasses, newClasses);
7066
toAdd = digestClassCounts(toAdd, 1);
7167
toRemove = digestClassCounts(toRemove, -1);
72-
if (toAdd && toAdd.length) {
73-
$animate.addClass(element, toAdd);
74-
}
75-
if (toRemove && toRemove.length) {
76-
$animate.removeClass(element, toRemove);
77-
}
68+
69+
attr.$addClass(toAdd);
70+
attr.$removeClass(toRemove);
7871
}
7972

8073
function ngClassWatchAction(newVal) {

‎test/ng/directive/ngClassSpec.js

+98-45
Original file line numberDiff line numberDiff line change
@@ -244,31 +244,42 @@ describe('ngClass', function() {
244244
}));
245245

246246

247-
it('should allow ngClassOdd/Even on the same element with overlapping classes', inject(function($rootScope, $compile, $animate) {
248-
var className;
249-
250-
element = $compile('<ul><li ng-repeat="i in [0,1,2]" ng-class-odd="\'same odd\'" ng-class-even="\'same even\'"></li><ul>')($rootScope);
247+
it('should allow ngClassOdd/Even on the same element with overlapping classes',
248+
inject(function($compile, $rootScope) {
249+
element = $compile(
250+
'<ul>' +
251+
'<li ng-repeat="i in [0,1,2]" ' +
252+
'ng-class-odd="\'same odd\'" ' +
253+
'ng-class-even="\'same even\'">' +
254+
'</li>' +
255+
'<ul>')($rootScope);
251256
$rootScope.$digest();
252-
var e1 = jqLite(element[0].childNodes[1]);
253-
var e2 = jqLite(element[0].childNodes[5]);
254-
expect(e1.hasClass('same')).toBeTruthy();
255-
expect(e1.hasClass('odd')).toBeTruthy();
256-
expect(e2.hasClass('same')).toBeTruthy();
257-
expect(e2.hasClass('odd')).toBeTruthy();
257+
258+
var e1 = element.children().eq(0);
259+
var e2 = element.children().eq(1);
260+
var e3 = element.children().eq(2);
261+
262+
expect(e1).toHaveClass('same');
263+
expect(e1).toHaveClass('odd');
264+
expect(e1).not.toHaveClass('even');
265+
expect(e2).toHaveClass('same');
266+
expect(e2).not.toHaveClass('odd');
267+
expect(e2).toHaveClass('even');
268+
expect(e3).toHaveClass('same');
269+
expect(e3).toHaveClass('odd');
270+
expect(e3).not.toHaveClass('even');
258271
})
259272
);
260273

261-
it('should allow ngClass with overlapping classes', inject(function($rootScope, $compile, $animate) {
274+
it('should allow ngClass with overlapping classes', inject(function($rootScope, $compile) {
262275
element = $compile('<div ng-class="{\'same yes\': test, \'same no\': !test}"></div>')($rootScope);
263276
$rootScope.$digest();
264277

265278
expect(element).toHaveClass('same');
266279
expect(element).not.toHaveClass('yes');
267280
expect(element).toHaveClass('no');
268281

269-
$rootScope.$apply(function() {
270-
$rootScope.test = true;
271-
});
282+
$rootScope.$apply('test = true');
272283

273284
expect(element).toHaveClass('same');
274285
expect(element).toHaveClass('yes');
@@ -300,37 +311,79 @@ describe('ngClass', function() {
300311
}));
301312

302313

303-
it('should reapply ngClass when interpolated class attribute changes', inject(function($rootScope, $compile) {
304-
element = $compile('<div class="one {{cls}} three" ng-class="{four: four}"></div>')($rootScope);
305-
306-
$rootScope.$apply(function() {
307-
$rootScope.cls = 'two';
308-
$rootScope.four = true;
309-
});
310-
expect(element).toHaveClass('one');
311-
expect(element).toHaveClass('two'); // interpolated
312-
expect(element).toHaveClass('three');
313-
expect(element).toHaveClass('four');
314-
315-
$rootScope.$apply(function() {
316-
$rootScope.cls = 'too';
317-
});
318-
expect(element).toHaveClass('one');
319-
expect(element).toHaveClass('too'); // interpolated
320-
expect(element).toHaveClass('three');
321-
expect(element).toHaveClass('four'); // should still be there
322-
expect(element.hasClass('two')).toBeFalsy();
323-
324-
$rootScope.$apply(function() {
325-
$rootScope.cls = 'to';
326-
});
327-
expect(element).toHaveClass('one');
328-
expect(element).toHaveClass('to'); // interpolated
329-
expect(element).toHaveClass('three');
330-
expect(element).toHaveClass('four'); // should still be there
331-
expect(element.hasClass('two')).toBeFalsy();
332-
expect(element.hasClass('too')).toBeFalsy();
333-
}));
314+
it('should reapply ngClass when interpolated class attribute changes',
315+
inject(function($compile, $rootScope) {
316+
element = $compile(
317+
'<div>' +
318+
'<div class="one {{two}} three" ng-class="{five: five}"></div>' +
319+
'<div class="one {{two}} three {{four}}" ng-class="{five: five}"></div>' +
320+
'</div>')($rootScope);
321+
var e1 = element.children().eq(0);
322+
var e2 = element.children().eq(1);
323+
324+
$rootScope.$apply('two = "two"; five = true');
325+
326+
expect(e1).toHaveClass('one');
327+
expect(e1).toHaveClass('two');
328+
expect(e1).toHaveClass('three');
329+
expect(e1).not.toHaveClass('four');
330+
expect(e1).toHaveClass('five');
331+
expect(e2).toHaveClass('one');
332+
expect(e2).toHaveClass('two');
333+
expect(e2).toHaveClass('three');
334+
expect(e2).not.toHaveClass('four');
335+
expect(e2).toHaveClass('five');
336+
337+
$rootScope.$apply('two = "another-two"');
338+
339+
expect(e1).toHaveClass('one');
340+
expect(e1).not.toHaveClass('two');
341+
expect(e1).toHaveClass('another-two');
342+
expect(e1).toHaveClass('three');
343+
expect(e1).not.toHaveClass('four');
344+
expect(e1).toHaveClass('five');
345+
expect(e2).toHaveClass('one');
346+
expect(e2).not.toHaveClass('two');
347+
expect(e2).toHaveClass('another-two');
348+
expect(e2).toHaveClass('three');
349+
expect(e2).not.toHaveClass('four');
350+
expect(e2).toHaveClass('five');
351+
352+
$rootScope.$apply('two = "two-more"; four = "four"');
353+
354+
expect(e1).toHaveClass('one');
355+
expect(e1).not.toHaveClass('two');
356+
expect(e1).not.toHaveClass('another-two');
357+
expect(e1).toHaveClass('two-more');
358+
expect(e1).toHaveClass('three');
359+
expect(e1).not.toHaveClass('four');
360+
expect(e1).toHaveClass('five');
361+
expect(e2).toHaveClass('one');
362+
expect(e2).not.toHaveClass('two');
363+
expect(e2).not.toHaveClass('another-two');
364+
expect(e2).toHaveClass('two-more');
365+
expect(e2).toHaveClass('three');
366+
expect(e2).toHaveClass('four');
367+
expect(e2).toHaveClass('five');
368+
369+
$rootScope.$apply('five = false');
370+
371+
expect(e1).toHaveClass('one');
372+
expect(e1).not.toHaveClass('two');
373+
expect(e1).not.toHaveClass('another-two');
374+
expect(e1).toHaveClass('two-more');
375+
expect(e1).toHaveClass('three');
376+
expect(e1).not.toHaveClass('four');
377+
expect(e1).not.toHaveClass('five');
378+
expect(e2).toHaveClass('one');
379+
expect(e2).not.toHaveClass('two');
380+
expect(e2).not.toHaveClass('another-two');
381+
expect(e2).toHaveClass('two-more');
382+
expect(e2).toHaveClass('three');
383+
expect(e2).toHaveClass('four');
384+
expect(e2).not.toHaveClass('five');
385+
})
386+
);
334387

335388

336389
it('should not mess up class value due to observing an interpolated class attribute', inject(function($rootScope, $compile) {

0 commit comments

Comments
 (0)