From defe39033a7840badfaad293c46ed1d09dd655cb Mon Sep 17 00:00:00 2001 From: Jeffrey Carl Faden Date: Wed, 17 Feb 2016 15:43:47 -0800 Subject: [PATCH 1/5] fix(directive): check for existing elements before appending Don't append elements that Masonry has already added to its items. --- src/angular-masonry.js | 5 ++++- test/mocks/masonry.mock.js | 3 ++- test/spec/directive.coffee | 18 ++++++++++++++++-- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/angular-masonry.js b/src/angular-masonry.js index 1ce8977..155efd3 100755 --- a/src/angular-masonry.js +++ b/src/angular-masonry.js @@ -65,7 +65,10 @@ // Keep track of added elements. bricks[id] = true; defaultLoaded(element); - $element.masonry(method, element, true); + // Don't add element to Masonry if it already has it. + if ($element.masonry('getItemElements').indexOf(element.get(0)) === -1) { + $element.masonry(method, element, true); + } } } diff --git a/test/mocks/masonry.mock.js b/test/mocks/masonry.mock.js index 5be3014..45e8298 100644 --- a/test/mocks/masonry.mock.js +++ b/test/mocks/masonry.mock.js @@ -1,4 +1,5 @@ (function () { 'use strict'; - $.fn.masonry = sinon.spy(); + $.fn.masonry = sinon.stub(); + $.fn.masonry.withArgs('getItemElements').returns([]); }()); diff --git a/test/spec/directive.coffee b/test/spec/directive.coffee index a89fd1e..2cebf31 100644 --- a/test/spec/directive.coffee +++ b/test/spec/directive.coffee @@ -177,8 +177,10 @@ describe 'angular-masonry', -> ''' element = $compile(element)(@scope) @scope.$digest() - # 2 is resize and one layout, 3 for the elements - expect($.fn.masonry.callCount).toBe(2 + 3) + # 2 is resize and one layout + # 3 is for checking whether the element is already added + # 3 is for appending the elements + expect($.fn.masonry.callCount).toBe(2 + 3 + 3) ) it 'should prepend elements when specified by attribute', inject(($compile) => @@ -248,3 +250,15 @@ describe 'angular-masonry', -> $timeout.flush() expect($.fn.masonry.calledWith('layout', sinon.match.any, sinon.match.any)).toBe(true) ) + + it 'should not append if masonry already has the element', inject(($compile) => + element = angular.element ''' + +
+ + ''' + $.fn.masonry.withArgs('getItemElements').returns indexOf: -> 0 + element = $compile(element)(@scope) + @scope.$digest() + expect($.fn.masonry.calledWith('appended', sinon.match.any, sinon.match.any)).toBe(false) + ) From 6e5572a6b1a04d24ae23f1693b91c7fd0caff318 Mon Sep 17 00:00:00 2001 From: Jeffrey Carl Faden Date: Fri, 19 Feb 2016 09:55:05 -0800 Subject: [PATCH 2/5] perf(directive): avoid O(n^2) iteration by setting existing elements as static Using jQuery's data functionality, we can mark static elements upon initialization. --- src/angular-masonry.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/angular-masonry.js b/src/angular-masonry.js index 155efd3..8f7d97d 100755 --- a/src/angular-masonry.js +++ b/src/angular-masonry.js @@ -66,7 +66,7 @@ bricks[id] = true; defaultLoaded(element); // Don't add element to Masonry if it already has it. - if ($element.masonry('getItemElements').indexOf(element.get(0)) === -1) { + if (element.data('angularMasonryStatic') !== true) { $element.masonry(method, element, true); } } @@ -134,6 +134,9 @@ columnWidth: parseInt(attrs.columnWidth, 10) || attrs.columnWidth }, attrOptions || {}); element.masonry(options); + element.children().each(function () { + $(this).data('angularMasonryStatic', true); + }); scope.masonryContainer = element[0]; var loadImages = scope.$eval(attrs.loadImages); ctrl.loadImages = loadImages !== false; From ff61b386d815716a81630209bdee5019edc18686 Mon Sep 17 00:00:00 2001 From: Jeffrey Carl Faden Date: Fri, 19 Feb 2016 09:58:25 -0800 Subject: [PATCH 3/5] fix(directive): use angular.element instead of $ Avoid using possibly non-existent global $. --- src/angular-masonry.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/angular-masonry.js b/src/angular-masonry.js index 8f7d97d..e82dc14 100755 --- a/src/angular-masonry.js +++ b/src/angular-masonry.js @@ -135,7 +135,7 @@ }, attrOptions || {}); element.masonry(options); element.children().each(function () { - $(this).data('angularMasonryStatic', true); + angular.element(this).data('angularMasonryStatic', true); }); scope.masonryContainer = element[0]; var loadImages = scope.$eval(attrs.loadImages); From 2d8532bbf827ef5288cee24ef1308ba2596620c8 Mon Sep 17 00:00:00 2001 From: Jeffrey Carl Faden Date: Wed, 24 Feb 2016 10:12:20 -0800 Subject: [PATCH 4/5] fix(spec): change specs to reflect use of ng-repeat Angular-masonry supports dynamic elements by default; Be explicit in the specs that most cases involve dynamically repeating bricks --- test/mocks/masonry.mock.js | 1 - test/spec/directive.coffee | 29 ++++++++++++++--------------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/test/mocks/masonry.mock.js b/test/mocks/masonry.mock.js index 45e8298..356522e 100644 --- a/test/mocks/masonry.mock.js +++ b/test/mocks/masonry.mock.js @@ -1,5 +1,4 @@ (function () { 'use strict'; $.fn.masonry = sinon.stub(); - $.fn.masonry.withArgs('getItemElements').returns([]); }()); diff --git a/test/spec/directive.coffee b/test/spec/directive.coffee index 2cebf31..0d867fc 100644 --- a/test/spec/directive.coffee +++ b/test/spec/directive.coffee @@ -132,12 +132,14 @@ describe 'angular-masonry', -> ) it 'should register an element in the parent controller', inject(($compile) => + @scope.bricks = [1] element = angular.element ''' -
+
''' element = $compile(element)(@scope) + @scope.$digest() # Needed for initial ng-repeat expect(@addBrick).toHaveBeenCalledOnce() ) @@ -161,32 +163,30 @@ describe 'angular-masonry', -> ) describe 'masonry-brick internals', => - beforeEach -> + beforeEach => $.fn.imagesLoaded = (cb) -> cb() + @scope.bricks = [1] afterEach -> delete $.fn.imagesLoaded it 'should append three elements to the controller', inject(($compile) => + @scope.bricks = [1, 2, 3] element = angular.element ''' -
-
-
+
''' element = $compile(element)(@scope) @scope.$digest() - # 2 is resize and one layout - # 3 is for checking whether the element is already added - # 3 is for appending the elements - expect($.fn.masonry.callCount).toBe(2 + 3 + 3) + # 2 is resize and one layout, 3 for the elements + expect($.fn.masonry.callCount).toBe(2 + 3) ) it 'should prepend elements when specified by attribute', inject(($compile) => element = angular.element ''' -
+
''' element = $compile(element)(@scope) @@ -198,7 +198,7 @@ describe 'angular-masonry', -> it 'should append before imagesLoaded when preserve-order is set', inject(($compile) => element = angular.element ''' -
+
''' imagesLoadedCb = undefined @@ -211,7 +211,7 @@ describe 'angular-masonry', -> it 'should call layout after imagesLoaded when preserve-order is set', inject(($compile, $timeout) => element = angular.element ''' -
+
''' imagesLoadedCb = undefined @@ -227,7 +227,7 @@ describe 'angular-masonry', -> it 'should append before imagesLoaded when load-images is set to "false"', inject(($compile) => element = angular.element ''' -
+
''' imagesLoadedCb = undefined @@ -240,7 +240,7 @@ describe 'angular-masonry', -> it 'should call layout before imagesLoaded when load-images is set to "false"', inject(($compile, $timeout) => element = angular.element ''' -
+
''' imagesLoadedCb = undefined @@ -257,7 +257,6 @@ describe 'angular-masonry', ->
''' - $.fn.masonry.withArgs('getItemElements').returns indexOf: -> 0 element = $compile(element)(@scope) @scope.$digest() expect($.fn.masonry.calledWith('appended', sinon.match.any, sinon.match.any)).toBe(false) From 141a66e7bce36be8d28c4ae3cb26f1674f6aff3a Mon Sep 17 00:00:00 2001 From: Jeffrey Carl Faden Date: Wed, 24 Feb 2016 10:31:54 -0800 Subject: [PATCH 5/5] refactor(spec): undo unnecessary change to mock file --- test/mocks/masonry.mock.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mocks/masonry.mock.js b/test/mocks/masonry.mock.js index 356522e..5be3014 100644 --- a/test/mocks/masonry.mock.js +++ b/test/mocks/masonry.mock.js @@ -1,4 +1,4 @@ (function () { 'use strict'; - $.fn.masonry = sinon.stub(); + $.fn.masonry = sinon.spy(); }());