Skip to content

Commit

Permalink
refactor(comments): code is now massively less hackish
Browse files Browse the repository at this point in the history 
At most a single optional comment controller is instantiated now, compared with
two from before.

The isolate scopes are now being made use of more correctly, rather than eval-
uating expressions from the parent scope. This simplifies the code somewhat, and
makes it considerably easier to grasp for beginners.

In general the library has been simplified somewhat, and should be that much more
effective from now on.

It is still desirable to make use of the sortBy attributes, so that more can be
done.
  • Loading branch information
Caitlin Potter committed Oct 22, 2013
1 parent 9355f72 commit f1d26b8
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 62 deletions.
69 changes: 30 additions & 39 deletions src/comments.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,9 @@ angular.module('ui.comments.directive', [])
}
}
function controllerSetter(setting, value) {
if (value && typeof value === 'string' ||
typeof value === 'function' ||
angular.isArray(value)) {
if (value && (angular.isString(value) && value.length ||
angular.isFunction(value) ||
angular.isArray(value))) {
config[setting] = value;
} else {
config[setting] = emptyController;
Expand Down Expand Up @@ -177,39 +177,35 @@ angular.module('ui.comments.directive', [])
* The comments container produces an isolate scope, and injected into the isolate scope is the
* the following:
*
* - _self_: Container for all exposed properties
* - _self.commentData_: Collection of comments, shared with the parent scope.
* - _comments_: Collection of comments, shared with the parent scope.
*
* **TODO**: Expose the child-container status in _self_, so that it may be ng-if'd in templates.
* **TODO**: Expose the child-container status in _status_, so that it may be ng-if'd in templates.
*
* The container should contain an `ng-repeat` directive for child comments. A very simple example
* of the {@link ui.comments.commentsConfig#containerTemplate containerTemplate} might look like
* the following:
*
* <pre>
* <div class="comments">
* <comment ng-repeat="comment in self.commentData" comment-data="comment"></comment>
* <comment ng-repeat="comment in comments" comment-data="comment"></comment>
* </div>
* </pre>
*/
.directive('comments', function($compile, commentsConfig) {
return {
restrict: 'EA',
require: '?^comment',
require: ['?^comment'],
transclude: true,
replace: true,
templateUrl: function() { return commentsConfig.containerTemplate; },
scope: true,
controller: function($scope) {},
scope: {
'comments': '=commentData'
},
controller: function() {},
compile: function() {
return function(scope, elem, attr, CommentsCtrl, CommentCtrl) {
var isDef = angular.isDefined, $eval = scope.$eval, children = false;
scope.self = {};
scope.$watchCollection(attr.commentData, function(newval, oldval) {
scope.self.commentData = angular.isArray(newval) ? newval : [];
});
return function(scope, elem, attr, ctrl) {
attr.$observe('orderBy', function(newval, oldval) {
scope.self.commentOrder = newval || commentsConfig.orderBy;
scope.commentOrder = newval || commentsConfig.orderBy;
});
};
}
Expand Down Expand Up @@ -259,53 +255,48 @@ angular.module('ui.comments.directive', [])
* infinite {@link http://docs.angularjs.org/api/ng.$compile $compile} loop, and eat a lot of
* memory.
*/
.directive('comment', function($compile, commentsConfig) {
.directive('comment', function($compile, commentsConfig, $controller) {
return {
require: '^comments',
restrict: 'EA',
transclude: true,
replace: true,
templateUrl: function() { return commentsConfig.commentTemplate; },
controller: function($scope, $controller, commentsConfig) {
var unregister = $scope.$watch('$element', function($element) {
unregister();
unregister = undefined;
var controller = commentsConfig.commentController,
controllerInstance;
if (controller) {
controllerInstance = $controller(controller, {
'$scope': $scope
});
if (controllerInstance) {
$element.data('$commentController', controllerInstance);
}
}
});
},
scope: {
comment: '=commentData'
},
compile: function(scope, elem) {
return function(scope, elem, attr, comments) {
var controller = commentsConfig.commentController, controllerInstance;
if (controller) {
controllerInstance = $controller(controller, {
'$scope': scope,
'$element': elem
});
if (controllerInstance) {
elem.data('$commentController', controllerInstance);
}
}
if (elem.parent().attr('child-comments') === 'true') {
elem.addClass('child-comment');
}
scope.comment = scope.$eval(attr.commentData);
var children = false, compiled, sub =
angular.element('<comments child-comments="true" ' +
'comment-data="comment.children"></comments>');
scope.$element = elem;
function update(data) {
if (angular.isArray(data) && data.length > 0 && !children) {
compiled = $compile(sub)(scope);
var w = scope.$watch('$$phase', function(newval) {
w();
scope.$element.append(compiled);
scope.$element.triggerHandler('filled.comments', compiled);
elem.append(compiled);
elem.triggerHandler('filled.comments', compiled);
children = true;
});
} else if((!angular.isArray(data) || !data.length) && children) {
children = false;
compiled.remove();
compiled = undefined;
scope.$element.triggerHandler('emptied.comments');
elem.triggerHandler('emptied.comments');
}
}

Expand Down
76 changes: 54 additions & 22 deletions src/test/comments.spec.js
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
describe('ui.comments', function() {
var $scope, $document, $body, $compile, comments;
var $scope, $document, $body, $compile, comments, firstComment, firstCtrl;
beforeEach(function() {
angular.module('testModule', [
'ui.comments.directive',
'template/comments/comments.html',
'template/comments/comment.html'
])
.controller('TestCtrl1', function($scope) {
$scope.controllerName = "TestCtrl1";
.controller('TestCtrl1', function($scope, $element) {
this.controllerName = "TestCtrl1";
this.$element = $element;
})
.controller('TestCtrl2', function($scope) {
$scope.controllerName = "TestCtrl2";
.controller('TestCtrl2', function($scope, $element) {
this.controllerName = "TestCtrl2";
this.$element = $element;
});
angular.forEach([
'testModule',
Expand All @@ -27,22 +29,31 @@ describe('ui.comments', function() {
$compile = _$compile_;
$body = $document.find('body');
});
firstComment = function() {
return comments.find('.comment').first();
}
firstCtrl = function() {
return firstComment().controller('comment');
}
});


describe('DOM', function() {
describe('top-level comments', function() {
beforeEach(function() {
$scope.comments = [];
comments = $compile(angular.element('<comments comment-data="comments"></comments>'))($scope);
comments = $compile('<comments comment-data="comments"></comments>')($scope);
});


it('adds comment to DOM when comment model grows', function() {
expect(comments.children().length).toEqual(0);
$scope.comments.push({});
$scope.$digest();
expect(comments.children().length).toEqual(1);
});


it('removes comment from DOM when comment model shrinks', function() {
$scope.comments.push({});
$scope.$digest();
Expand All @@ -52,6 +63,7 @@ describe('ui.comments', function() {
expect(comments.children().length).toEqual(0);
});


it('changes comment data when comment model changes', function() {
$scope.comments.push({text: 'Test Comment'});
$scope.$digest();
Expand All @@ -61,6 +73,7 @@ describe('ui.comments', function() {
expect(comments.find('.comment-body').text()).toEqual("Changed Comment");
});


it('re-orders comments in DOM when comment model is re-ordered', function() {
$scope.comments.push({text: '123'});
$scope.comments.push({text: 'ABC'});
Expand All @@ -72,6 +85,7 @@ describe('ui.comments', function() {
});
});


describe('child comments', function() {
beforeEach(function() {
$scope.comments = [{
Expand All @@ -81,24 +95,27 @@ describe('ui.comments', function() {
{ text: 'Second child' }
]
}];
comments = $compile(angular.element('<comments comment-data="comments"></comments>'))($scope);
comments = $compile('<comments comment-data="comments"></comments>')($scope);
$scope.$digest();
});


it('adds comment to DOM when child comment model grows', function() {
expect(comments.find('.child-comment').length).toEqual(2);
$scope.comments[0].children.push({text: 'Test'});
$scope.$digest();
expect(comments.find('.child-comment').length).toEqual(3);
});


it('removes comment from DOM when child comment model shrinks', function() {
expect(comments.find('.child-comment').length).toEqual(2);
$scope.comments[0].children.pop();
$scope.$digest();
expect(comments.find('.child-comment').length).toEqual(1);
});


it('changes comment data when child comment model changes', function() {
var first = comments.find('.child-comment > .comment-body').first();
expect(first.text()).toEqual("First child");
Expand All @@ -107,6 +124,7 @@ describe('ui.comments', function() {
expect(first.text()).toEqual("Changed Comment");
});


it('re-orders comments in DOM when child comment model is re-ordered', function() {
var children = comments.find('.child-comment');
expect(children.find('.comment-body').first().text()).toEqual('First child');
Expand All @@ -117,10 +135,11 @@ describe('ui.comments', function() {
});
});


describe('events', function() {
it('fires `comments.filled` when child comments become available', function() {
$scope.comments = [{children: []}];
comments = $compile(angular.element('<comments comment-data="comments"></comments>'))($scope);
comments = $compile('<comments comment-data="comments"></comments>')($scope);
$scope.$digest();
var parent = comments.find('.comment').first(),
callback = jasmine.createSpy('commentsFilled');
Expand All @@ -130,9 +149,10 @@ describe('ui.comments', function() {
expect(callback).toHaveBeenCalledWith(jasmine.any(Object), jasmine.any(HTMLDivElement));
});


it('fires `comments.emptied` when child comments are no longer available', function() {
$scope.comments = [{children: [{}]}];
comments = $compile(angular.element('<comments comment-data="comments"></comments>'))($scope);
comments = $compile('<comments comment-data="comments"></comments>')($scope);
$scope.$digest();
var parent = comments.find('.comment').first(),
callback = jasmine.createSpy('commentsEmptied');
Expand All @@ -143,6 +163,7 @@ describe('ui.comments', function() {
});
});


describe('custom comment controller', function() {
var commentsConfig;
beforeEach(function() {
Expand All @@ -154,31 +175,42 @@ describe('ui.comments', function() {

it('instantiates the controller named in commentConfig', function() {
commentsConfig.commentController = 'TestCtrl1';
comments = $compile(angular.element('<comments comment-data="comments"></comments>'))($scope);
comments = $compile('<comments comment-data="comments"></comments>')($scope);
$scope.$digest();
expect(comments.find('.comment').first().scope().controllerName).toEqual('TestCtrl1');
});
it('instantiates the controller named in commentConfig', function() {
expect(firstCtrl().controllerName).toEqual('TestCtrl1');

commentsConfig.commentController = 'TestCtrl2';
comments = $compile(angular.element('<comments comment-data="comments"></comments>'))($scope);
comments = $compile('<comments comment-data="comments"></comments>')($scope);
$scope.$digest();
expect(comments.find('.comment').first().scope().controllerName).toEqual('TestCtrl2');
expect(firstCtrl().controllerName).toEqual('TestCtrl2');
});


it('instantiates the controller function in commentConfig', function() {
commentsConfig.commentController = function($scope) {
$scope.controllerName = 'TestCtrl3';
this.controllerName = 'TestCtrl3';
};
comments = $compile(angular.element('<comments comment-data="comments"></comments>'))($scope);
comments = $compile('<comments comment-data="comments"></comments>')($scope);
$scope.$digest();
expect(comments.find('.comment').first().scope().controllerName).toEqual('TestCtrl3');
expect(firstCtrl().controllerName).toEqual('TestCtrl3');
});
it('instantiates the controller function in commentConfig with bracket notation', function() {


it('instantiates the controller function in commentConfig with array annotation', function() {
commentsConfig.commentController = ['$scope', function(s) {
s.controllerName = 'TestCtrl4';
this.controllerName = 'TestCtrl4';
}];
comments = $compile(angular.element('<comments comment-data="comments"></comments>'))($scope);
comments = $compile('<comments comment-data="comments"></comments>')($scope);
$scope.$digest();
expect(comments.find('.comment').first().scope().controllerName).toEqual('TestCtrl4');
expect(firstCtrl().controllerName).toEqual('TestCtrl4');
});


it('injects the comment $element into controller', function() {
commentsConfig.commentController = 'TestCtrl2';
comments = $compile('<comments comment-data="comments"></comments>')($scope);
$scope.$digest();
expect(firstComment()[0]).toEqual(firstCtrl().$element[0]);
});
});
});
2 changes: 1 addition & 1 deletion template/comments/comments.html
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<div class="comments">
<comment ng-repeat="comment in self.commentData" comment-data="comment"></comment>
<comment ng-repeat="comment in comments" comment-data="comment"></comment>
</div>

0 comments on commit f1d26b8

Please sign in to comment.