Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Data List Directive with ngdocs and unit tests #45

Merged
merged 1 commit into from
Aug 27, 2015

Conversation

jeff-phillips-18
Copy link
Member

Also adds a utility transclude directive to allow isolated scopes on
items inside a directive.

@@ -0,0 +1,46 @@

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ngdoc. I'm not sure unit tests are applicable here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wouldn't they be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just not entirely sure how to test this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd just test each type of transclusion for the correct scope and DOM structure. Should be pretty similar to other directive tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dtaylor113
Copy link
Member

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',
function ($scope) {
$scope.eventText = '';
var handleSelect = function (item, e) {
...
...
SyntaxError: Unexpected token ;
at eval (native)
at http://localhost:8000/js/angular-bootstrap-prettify.js:179:20
at Object.forEach (http://localhost:8000/grunt-scripts/angular.js:326:20)
at Object.compile (http://localhost:8000/js/angular-bootstrap-prettify.js:173:15)
at applyDirectivesToNode (http://localhost:8000/grunt-scripts/angular.js:7530:32)
...
...
Error: [ng:areq] Argument 'ViewCtrl' is not a function, got undefined
http://errors.angularjs.org/1.3.18/ng/areq?p0=ViewCtrl&p1=not%20a%20function%2C%20got%20undefined
at REGEX_STRING_REGEXP (angular.js:63)
at assertArg (angular.js:1590)
at assertArgFn (angular.js:1600)

Not sure if this is my enviornment or something in the repo?

~/Dev/repos/j-a-pf>bower list
bower check-new Checking for new versions of the project dependencies...
angular-patternfly#2.0.0 /home/dtaylor/Dev/repos/j-a-pf
├── angular#1.3.18 (latest is 1.4.4)
├─┬ angular-animate#1.3.18 (latest is 1.4.4)
│ └── angular#1.3.18 (latest is 1.4.4)
├─┬ angular-mocks#1.3.18 (latest is 1.4.4)
│ └── angular#1.3.18
├─┬ angular-route#1.3.18 (latest is 1.4.4)
│ └── angular#1.3.18
├─┬ angular-sanitize#1.3.18 (latest is 1.4.4)
│ └── angular#1.3.18
├─┬ angular-touch#1.3.18 (latest is 1.4.4)
│ └── angular#1.3.18
├── lodash#3.10.1
└── #2.0.0

items: '=',
eventId: '@id'
},
replace: true,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, removed it.

@jeff-phillips-18
Copy link
Member Author

Dave, sorry about that. It was a merge issue when I rebased with Angular 1.3. Fix is coming soon.

@dtaylor113
Copy link
Member

Ok, thanks, LGTM

.directive( 'transcludeSibling', function() {
return {
restrict: 'E',
transclude: true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why these are restricted to just 'E'? normal transcludes support both

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason. Just only necessary here based on how the example is written.

@jeff-phillips-18 jeff-phillips-18 force-pushed the master branch 2 times, most recently from 532d7a8 to 53baee6 Compare August 27, 2015 14:09
@jeff-phillips-18
Copy link
Member Author

Squashed and rebased.

@jeff-phillips-18
Copy link
Member Author

@waldenraines @erundle Can this be merged?

@erundle
Copy link

erundle commented Aug 27, 2015

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

erundle pushed a commit that referenced this pull request Aug 27, 2015
Add Data List Directive with ngdocs and unit tests
@erundle erundle merged commit adbde2e into patternfly:master Aug 27, 2015
@waldenraines
Copy link

@waldenraines @erundle Can this be merged?

Can we create a GH issue for adding the unit tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants