Add Data List Directive with ngdocs and unit tests #45
Conversation
| @@ -0,0 +1,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
This is lacking documentation and tests.
Also, you may want to mention the AngularJS issue number (angular/angular.js#5489) that this is monkey patching in said documentation so we can follow that issue and know when we can get rid of this.
There was a problem hiding this comment.
Added ngdoc. I'm not sure unit tests are applicable here.
There was a problem hiding this comment.
Just not entirely sure how to test this.
There was a problem hiding this comment.
You'd just test each type of transclusion for the correct scope and DOM structure. Should be pretty similar to other directive tests.
There was a problem hiding this comment.
I'm struggling with creating directives on the fly in the test spec which is what I need to do in order to test the scope and DOM structures based on the transclusion type. The example in ng-doc show this and tests this pretty well.
@waldenraines: I'd like to defer this. We can add an issue and I will address when I can. I need to get a 2.1 label on angulary-patternfly for today as there are folks that need the latest today and I'd like for the data list to be included.
|
Hi, I just cloned your repo, npm install, bower install, grunt build, grunt ngdocs:view and when I click on the pfDataList ngdoc, I get: angular.module('patternfly.views').controller('ViewCtrl', ['$scope', Not sure if this is my enviornment or something in the repo? ~/Dev/repos/j-a-pf>bower list |
| items: '=', | ||
| eventId: '@id' | ||
| }, | ||
| replace: true, |
There was a problem hiding this comment.
Thanks, removed it.
|
Dave, sorry about that. It was a merge issue when I rebased with Angular 1.3. Fix is coming soon. |
jeff-phillips-18
force-pushed
the
master
branch
from
d7a4321
to
7a9441b
Compare
|
Ok, thanks, LGTM |
| .directive( 'transcludeSibling', function() { | ||
| return { | ||
| restrict: 'E', | ||
| transclude: true, |
There was a problem hiding this comment.
Any reason why these are restricted to just 'E'? normal transcludes support both
There was a problem hiding this comment.
No reason. Just only necessary here based on how the example is written.
jeff-phillips-18
force-pushed
the
master
branch
2 times, most recently
from
532d7a8
to
53baee6
Compare
|
Squashed and rebased. |
jeff-phillips-18
force-pushed
the
master
branch
from
53baee6
to
74eb9f7
Compare
|
@waldenraines @erundle Can this be merged? |
|
I am fine with approach pending you do investigate writing additional unit test later today/monday. They have proven very useful when upgrading components etc and is something that is need. We have already had a request to support 1.4.x |
Add Data List Directive with ngdocs and unit tests
Can we create a GH issue for adding the unit tests? |
Also adds a utility transclude directive to allow isolated scopes on
items inside a directive.