Skip to content

Commit 0409589

Browse files
committed
refactor(ngClass): remove redundant $observer
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 8822006 commit 0409589

File tree

2 files changed

+72
-35
lines changed

2 files changed

+72
-35
lines changed

src/ng/directive/ngClass.js

-5
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,6 @@ function classDirective(name, selector) {
1717

1818
scope.$watch(attr[name], ngClassWatchAction, true);
1919

20-
attr.$observe('class', function(value) {
21-
ngClassWatchAction(scope.$eval(attr[name]));
22-
});
23-
24-
2520
if (name !== 'ngClass') {
2621
scope.$watch('$index', function($index, old$index) {
2722
/* eslint-disable no-bitwise */

test/ng/directive/ngClassSpec.js

+72-30
Original file line numberDiff line numberDiff line change
@@ -311,37 +311,79 @@ fdescribe('ngClass', function() {
311311
}));
312312

313313

314-
it('should reapply ngClass when interpolated class attribute changes', inject(function($rootScope, $compile) {
315-
element = $compile('<div class="one {{cls}} three" ng-class="{four: four}"></div>')($rootScope);
316-
317-
$rootScope.$apply(function() {
318-
$rootScope.cls = 'two';
319-
$rootScope.four = true;
320-
});
321-
expect(element).toHaveClass('one');
322-
expect(element).toHaveClass('two'); // interpolated
323-
expect(element).toHaveClass('three');
324-
expect(element).toHaveClass('four');
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);
325323

326-
$rootScope.$apply(function() {
327-
$rootScope.cls = 'too';
328-
});
329-
expect(element).toHaveClass('one');
330-
expect(element).toHaveClass('too'); // interpolated
331-
expect(element).toHaveClass('three');
332-
expect(element).toHaveClass('four'); // should still be there
333-
expect(element.hasClass('two')).toBeFalsy();
334-
335-
$rootScope.$apply(function() {
336-
$rootScope.cls = 'to';
337-
});
338-
expect(element).toHaveClass('one');
339-
expect(element).toHaveClass('to'); // interpolated
340-
expect(element).toHaveClass('three');
341-
expect(element).toHaveClass('four'); // should still be there
342-
expect(element.hasClass('two')).toBeFalsy();
343-
expect(element.hasClass('too')).toBeFalsy();
344-
}));
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 = "too"');
338+
339+
expect(e1).toHaveClass('one');
340+
expect(e1).not.toHaveClass('two');
341+
expect(e1).toHaveClass('too');
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('too');
348+
expect(e2).toHaveClass('three');
349+
expect(e2).not.toHaveClass('four');
350+
expect(e2).toHaveClass('five');
351+
352+
$rootScope.$apply('two = "to"; four = "four"');
353+
354+
expect(e1).toHaveClass('one');
355+
expect(e1).not.toHaveClass('two');
356+
expect(e1).not.toHaveClass('too');
357+
expect(e1).toHaveClass('to');
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('too');
364+
expect(e2).toHaveClass('to');
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('too');
374+
expect(e1).toHaveClass('to');
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('too');
381+
expect(e2).toHaveClass('to');
382+
expect(e2).toHaveClass('three');
383+
expect(e2).toHaveClass('four');
384+
expect(e2).not.toHaveClass('five');
385+
})
386+
);
345387

346388

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

0 commit comments

Comments
 (0)