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

Commit e2663f6

Browse files
dmanekIgorMinar
authored andcommitted
feat(ng:style): compatibility + perf improvements
- better compatibility with 3rd party code - we clober 3rd party style only if it direcrtly collides with 3rd party styles - better perf since it doesn't execute stuff on every digest - lots of tests
1 parent 9f9ed4c commit e2663f6

File tree

2 files changed

+68
-31
lines changed

2 files changed

+68
-31
lines changed

src/directives.js

+5-15
Original file line numberDiff line numberDiff line change
@@ -802,21 +802,11 @@ angularDirective("ng:hide", function(expression, element){
802802
</doc:scenario>
803803
</doc:example>
804804
*/
805-
angularDirective("ng:style", function(expression, element){
806-
// TODO(i): this is inefficient (runs on every $digest) and obtrusive (overrides 3rd part css)
807-
// we should change it in a similar way as I changed ng:class
808-
return function(element){
809-
var resetStyle = getStyle(element);
810-
this.$watch(function(scope){
811-
var style = scope.$eval(expression) || {}, key, mergedStyle = {};
812-
for(key in style) {
813-
if (resetStyle[key] === undefined) resetStyle[key] = '';
814-
mergedStyle[key] = style[key];
815-
}
816-
for(key in resetStyle) {
817-
mergedStyle[key] = mergedStyle[key] || resetStyle[key];
818-
}
819-
element.css(mergedStyle);
805+
angularDirective("ng:style", function(expression, element) {
806+
return function(element) {
807+
this.$watch(expression, function(scope, newStyles, oldStyles) {
808+
if (oldStyles) forEach(oldStyles, function(val, style) { element.css(style, '');});
809+
if (newStyles) element.css(newStyles);
820810
});
821811
};
822812
});

test/directivesSpec.js

+63-16
Original file line numberDiff line numberDiff line change
@@ -363,35 +363,82 @@ describe("directive", function() {
363363

364364

365365
describe('ng:style', function() {
366+
366367
it('should set', function() {
367368
var scope = compile('<div ng:style="{height: \'40px\'}"></div>');
368369
scope.$digest();
369370
expect(element.css('height')).toEqual('40px');
370371
});
371372

373+
372374
it('should silently ignore undefined style', function() {
373375
var scope = compile('<div ng:style="myStyle"></div>');
374376
scope.$digest();
375377
expect(element.hasClass('ng-exception')).toBeFalsy();
376378
});
377379

378-
it('should preserve and remove previous style', function() {
379-
var scope = compile('<div style="height: 10px;" ng:style="myStyle"></div>');
380-
scope.$digest();
381-
expect(getStyle(element)).toEqual({height: '10px'});
382-
scope.myStyle = {height: '20px', width: '10px'};
383-
scope.$digest();
384-
expect(getStyle(element)).toEqual({height: '20px', width: '10px'});
385-
scope.myStyle = {};
386-
scope.$digest();
387-
expect(getStyle(element)).toEqual({height: '10px'});
388-
});
389-
});
390380

391-
it('should silently ignore undefined ng:style', function() {
392-
var scope = compile('<div ng:style="myStyle"></div>');
393-
scope.$digest();
394-
expect(element.hasClass('ng-exception')).toBeFalsy();
381+
describe('preserving styles set before and after compilation', function() {
382+
var scope, preCompStyle, preCompVal, postCompStyle, postCompVal;
383+
384+
beforeEach(function() {
385+
preCompStyle = 'width';
386+
preCompVal = '300px';
387+
postCompStyle = 'height';
388+
postCompVal = '100px';
389+
element = jqLite('<div ng:style="styleObj"></div>');
390+
element.css(preCompStyle, preCompVal);
391+
jqLite(document.body).append(element);
392+
scope = compile(element);
393+
scope.styleObj = {'margin-top': '44px'};
394+
scope.$apply();
395+
element.css(postCompStyle, postCompVal);
396+
});
397+
398+
afterEach(function() {
399+
element.remove();
400+
});
401+
402+
403+
it('should not mess up stuff after compilation', function() {
404+
element.css('margin', '44px');
405+
expect(element.css(preCompStyle)).toBe(preCompVal);
406+
expect(element.css('margin-top')).toBe('44px');
407+
expect(element.css(postCompStyle)).toBe(postCompVal);
408+
});
409+
410+
411+
it('should not mess up stuff after $apply with no model changes', function() {
412+
element.css('padding-top', '33px');
413+
scope.$apply();
414+
expect(element.css(preCompStyle)).toBe(preCompVal);
415+
expect(element.css('margin-top')).toBe('44px');
416+
expect(element.css(postCompStyle)).toBe(postCompVal);
417+
expect(element.css('padding-top')).toBe('33px');
418+
});
419+
420+
421+
it('should not mess up stuff after $apply with non-colliding model changes', function() {
422+
scope.styleObj = {'padding-top': '99px'};
423+
scope.$apply();
424+
expect(element.css(preCompStyle)).toBe(preCompVal);
425+
expect(element.css('margin-top')).not.toBe('44px');
426+
expect(element.css('padding-top')).toBe('99px');
427+
expect(element.css(postCompStyle)).toBe(postCompVal);
428+
});
429+
430+
431+
it('should overwrite original styles after a colliding model change', function() {
432+
scope.styleObj = {'height': '99px', 'width': '88px'};
433+
scope.$apply();
434+
expect(element.css(preCompStyle)).toBe('88px');
435+
expect(element.css(postCompStyle)).toBe('99px');
436+
scope.styleObj = {};
437+
scope.$apply();
438+
expect(element.css(preCompStyle)).not.toBe('88px');
439+
expect(element.css(postCompStyle)).not.toBe('99px');
440+
});
441+
});
395442
});
396443

397444

0 commit comments

Comments
 (0)