From 40479063cce70be898c29c8cbd0c2441e7a125be Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Fri, 4 Nov 2011 17:13:56 -0700 Subject: [PATCH 1/5] Improve ng:style --- src/directives.js | 21 ++++++------ test/directivesSpec.js | 76 ++++++++++++++++++++++++++++++++---------- 2 files changed, 68 insertions(+), 29 deletions(-) diff --git a/src/directives.js b/src/directives.js index b9779adf17c2..a221040f77d9 100644 --- a/src/directives.js +++ b/src/directives.js @@ -789,19 +789,18 @@ angularDirective("ng:hide", function(expression, element){ */ angularDirective("ng:style", function(expression, element){ - // TODO(i): this is inefficient (runs on every $digest) and obtrusive (overrides 3rd part css) - // we should change it in a similar way as I changed ng:class return function(element){ - var resetStyle = getStyle(element); - this.$watch(function(scope){ - var style = scope.$eval(expression) || {}, key, mergedStyle = {}; - for(key in style) { - if (resetStyle[key] === undefined) resetStyle[key] = ''; - mergedStyle[key] = style[key]; - } - for(key in resetStyle) { - mergedStyle[key] = mergedStyle[key] || resetStyle[key]; + this.$watch(expression, function(scope, newVal, oldVal){ + var styleToAdd = newVal || {}, + styleToRemove = oldVal || {}, + key, + mergedStyle = {}; + for(key in styleToAdd) { + if (styleToAdd[key] !== undefined) + mergedStyle[key] = styleToAdd[key]; } + for(key in styleToRemove) + delete mergedStyle[key]; element.css(mergedStyle); }); }; diff --git a/test/directivesSpec.js b/test/directivesSpec.js index 8c07cf70a183..a82ddfaca0f2 100644 --- a/test/directivesSpec.js +++ b/test/directivesSpec.js @@ -362,7 +362,8 @@ describe("directive", function() { }); - describe('ng:style', function() { + ddescribe('ng:style', function() { + it('should set', function() { var scope = compile('
'); scope.$digest(); @@ -375,26 +376,65 @@ describe("directive", function() { expect(element.hasClass('ng-exception')).toBeFalsy(); }); - it('should preserve and remove previous style', function() { - var scope = compile('
'); - scope.$digest(); - expect(getStyle(element)).toEqual({height: '10px'}); - scope.myStyle = {height: '20px', width: '10px'}; - scope.$digest(); - expect(getStyle(element)).toEqual({height: '20px', width: '10px'}); - scope.myStyle = {}; - scope.$digest(); - expect(getStyle(element)).toEqual({height: '10px'}); + describe('preserving styles set before and after compilation', function() { + var scope, preCompStyle, preCompVal, postCompStyle, postCompVal; + + beforeEach(function() { + preCompStyle = 'width'; + preCompVal = '300px'; + postCompStyle = 'height'; + postCompVal = '100px'; + element = jqLite('
'); + element.css(preCompStyle, preCompVal); + jqLite(document.body).append(element); + scope = compile(element); + scope.styleObj = {'margin-top': '44px'}; + scope.$apply(); + element.css(postCompStyle, postCompVal); + }); + + afterEach(function() { + element.remove(); + }); + + it('should not mess up stuff after compilation', function() { + element.css('margin', '44px'); + expect(element.css(preCompStyle)).toBe(preCompVal); + expect(element.css('margin-top')).toBe('44px'); + expect(element.css(postCompStyle)).toBe(postCompVal); + }); + + it('should not mess up stuff after $apply with no model changes', function() { + element.css('padding-top', '33px'); + scope.$apply(); + expect(element.css(preCompStyle)).toBe(preCompVal); + expect(element.css('margin-top')).toBe('44px'); + expect(element.css(postCompStyle)).toBe(postCompVal); + expect(element.css('padding-top')).toBe('33px'); + }); + + it('should not mess up stuff after $apply with non-colliding model changes', function() { + scope.styleObj = {'padding-top': '99px'}; + scope.$apply(); + expect(element.css(preCompStyle)).toBe(preCompVal); + expect(element[0].style['margin-top']).toBe(''); // jquery doesn't return the dom value, so we go to dom + expect(element.css('padding-top')).toBe('99px'); + expect(element.css(postCompStyle)).toBe(postCompVal); + }); + + iit('should overwrite original styles after a colliding model change', function() { + scope.styleObj = {'height': '99px', 'width': '88px'}; + scope.$apply(); + expect(element.css(preCompStyle)).toBe('88px'); + expect(element.css(postCompStyle)).toBe('99px'); + scope.styleObj = {}; + scope.$apply(); + expect(element.css(preCompStyle)).toBe('auto'); // likely will need to go to the DOM because jquery makes shit up + expect(element.css(postCompStyle)).toBe('0px'); + }); }); }); - it('should silently ignore undefined ng:style', function() { - var scope = compile('
'); - scope.$digest(); - expect(element.hasClass('ng-exception')).toBeFalsy(); - }); - - describe('ng:show', function() { it('should show and hide an element', function() { var element = jqLite('
'), From aa06b3f16a763b023ea2a85d4beae6b446691502 Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Sun, 6 Nov 2011 14:07:49 -0800 Subject: [PATCH 2/5] fix(directives): improve ng:style --- src/directives.js | 11 +++-------- test/directivesSpec.js | 8 ++++---- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/directives.js b/src/directives.js index a221040f77d9..dfc15d04b145 100644 --- a/src/directives.js +++ b/src/directives.js @@ -793,15 +793,10 @@ angularDirective("ng:style", function(expression, element){ this.$watch(expression, function(scope, newVal, oldVal){ var styleToAdd = newVal || {}, styleToRemove = oldVal || {}, - key, - mergedStyle = {}; - for(key in styleToAdd) { - if (styleToAdd[key] !== undefined) - mergedStyle[key] = styleToAdd[key]; - } + key; for(key in styleToRemove) - delete mergedStyle[key]; - element.css(mergedStyle); + element.css(key, ''); + element.css(styleToAdd) }); }; }); diff --git a/test/directivesSpec.js b/test/directivesSpec.js index a82ddfaca0f2..f53ae48d5e36 100644 --- a/test/directivesSpec.js +++ b/test/directivesSpec.js @@ -362,7 +362,7 @@ describe("directive", function() { }); - ddescribe('ng:style', function() { + describe('ng:style', function() { it('should set', function() { var scope = compile('
'); @@ -422,15 +422,15 @@ describe("directive", function() { expect(element.css(postCompStyle)).toBe(postCompVal); }); - iit('should overwrite original styles after a colliding model change', function() { + it('should overwrite original styles after a colliding model change', function() { scope.styleObj = {'height': '99px', 'width': '88px'}; scope.$apply(); expect(element.css(preCompStyle)).toBe('88px'); expect(element.css(postCompStyle)).toBe('99px'); scope.styleObj = {}; scope.$apply(); - expect(element.css(preCompStyle)).toBe('auto'); // likely will need to go to the DOM because jquery makes shit up - expect(element.css(postCompStyle)).toBe('0px'); + expect(element.css(preCompStyle)).toBe(''); + expect(element.css(postCompStyle)).toBe(''); }); }); }); From 79906267cdd94c54cda3a9cf40528891141d7cc9 Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Sun, 6 Nov 2011 14:14:33 -0800 Subject: [PATCH 3/5] fix(directives): improve ng:style --- src/directives.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/directives.js b/src/directives.js index dfc15d04b145..430020fc1edf 100644 --- a/src/directives.js +++ b/src/directives.js @@ -796,7 +796,7 @@ angularDirective("ng:style", function(expression, element){ key; for(key in styleToRemove) element.css(key, ''); - element.css(styleToAdd) + element.css(styleToAdd); }); }; }); From 4368992f8e5bd06923cb369b73664f0cb88813e2 Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Sun, 6 Nov 2011 23:55:20 -0800 Subject: [PATCH 4/5] fix(directives): improve ng:style --- test/directivesSpec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/directivesSpec.js b/test/directivesSpec.js index f53ae48d5e36..96b316d0348a 100644 --- a/test/directivesSpec.js +++ b/test/directivesSpec.js @@ -396,7 +396,7 @@ describe("directive", function() { afterEach(function() { element.remove(); }); - + it('should not mess up stuff after compilation', function() { element.css('margin', '44px'); expect(element.css(preCompStyle)).toBe(preCompVal); @@ -417,10 +417,10 @@ describe("directive", function() { scope.styleObj = {'padding-top': '99px'}; scope.$apply(); expect(element.css(preCompStyle)).toBe(preCompVal); - expect(element[0].style['margin-top']).toBe(''); // jquery doesn't return the dom value, so we go to dom + expect(element.css('margin-top')).toBe(''); expect(element.css('padding-top')).toBe('99px'); expect(element.css(postCompStyle)).toBe(postCompVal); - }); + }); it('should overwrite original styles after a colliding model change', function() { scope.styleObj = {'height': '99px', 'width': '88px'}; From d45f26acb2850d79a2d910bb70873e21fed431c1 Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Mon, 7 Nov 2011 17:59:47 -0800 Subject: [PATCH 5/5] fix(directives): improve ng:style --- src/directives.js | 1 - test/directivesSpec.js | 14 ++++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/directives.js b/src/directives.js index 430020fc1edf..81a532a86336 100644 --- a/src/directives.js +++ b/src/directives.js @@ -801,7 +801,6 @@ angularDirective("ng:style", function(expression, element){ }; }); - /** * @ngdoc directive * @name angular.directive.ng:cloak diff --git a/test/directivesSpec.js b/test/directivesSpec.js index 96b316d0348a..5e795d2dd711 100644 --- a/test/directivesSpec.js +++ b/test/directivesSpec.js @@ -370,6 +370,7 @@ describe("directive", function() { expect(element.css('height')).toEqual('40px'); }); + it('should silently ignore undefined style', function() { var scope = compile('
'); scope.$digest(); @@ -397,6 +398,7 @@ describe("directive", function() { element.remove(); }); + it('should not mess up stuff after compilation', function() { element.css('margin', '44px'); expect(element.css(preCompStyle)).toBe(preCompVal); @@ -404,6 +406,7 @@ describe("directive", function() { expect(element.css(postCompStyle)).toBe(postCompVal); }); + it('should not mess up stuff after $apply with no model changes', function() { element.css('padding-top', '33px'); scope.$apply(); @@ -413,15 +416,17 @@ describe("directive", function() { expect(element.css('padding-top')).toBe('33px'); }); + it('should not mess up stuff after $apply with non-colliding model changes', function() { scope.styleObj = {'padding-top': '99px'}; scope.$apply(); expect(element.css(preCompStyle)).toBe(preCompVal); - expect(element.css('margin-top')).toBe(''); + expect(element.css('margin-top')).not.toBe('44px'); expect(element.css('padding-top')).toBe('99px'); expect(element.css(postCompStyle)).toBe(postCompVal); }); + it('should overwrite original styles after a colliding model change', function() { scope.styleObj = {'height': '99px', 'width': '88px'}; scope.$apply(); @@ -429,12 +434,13 @@ describe("directive", function() { expect(element.css(postCompStyle)).toBe('99px'); scope.styleObj = {}; scope.$apply(); - expect(element.css(preCompStyle)).toBe(''); - expect(element.css(postCompStyle)).toBe(''); - }); + expect(element.css(preCompStyle)).not.toBe('88px'); + expect(element.css(postCompStyle)).not.toBe('99px'); + }); }); }); + describe('ng:show', function() { it('should show and hide an element', function() { var element = jqLite('
'),