-
Notifications
You must be signed in to change notification settings - Fork 90
Convert sort, filter, and toolbar modules directives to components #376
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
Convert sort, filter, and toolbar modules directives to components #376
Conversation
}; | ||
if (!filterExists(newFilter)) { | ||
ctrl.config.filterConfig.appliedFilters.push(newFilter); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed here (this is an existing bug) that you don't have:
if (newFilter.type === 'select') {
enforceSingleSelect(newFilter);
}
Like you do for pfFilter. So in the pfToolbar example, the birth month dropdown is accumulative, whereas in the pfFilter it replaces. Not a biggie :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks.
addFilter: addFilter | ||
} | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the use of angular.extend to add functions to ctrl, I'm going to use that in my charts
@@ -19,8 +18,6 @@ describe('Directive: pfFilter', function () { | |||
var el = $compile(element)(scope); | |||
|
|||
scope.$digest(); | |||
|
|||
isoScope = el.isolateScope(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I've had success replace isoScope with the following:
- isoScope = el.isolateScope();
+ isoScope = el.controller('pfDonutPctChart');
Where 'pfDonutPctChart' is the name of the component controller your after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The restrict attribute needs to be added to the top of any converted documentation and I still notice $scope in the components. Will this be removed in a subsequent PR?
/** | ||
* @ngdoc directive | ||
* @name patternfly.filters.component:pfFilterFields | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe should this have a restrict E here to have the docs show the component correctly in the example code (instead of <ANY ...
}; | ||
|
||
ctrl.$postLink = function () { | ||
$scope.$watch('config', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to refactor this as this component can't be used within Angular2 - will that be done in a separate PR so we can move this one forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is $postlink not supported in Angular2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been working on a separate PR to remove the $scope references, its a bit more work and I'd rather not try to address in this PR.
@dtaylor113 $postLink is OK, it's the use of $scope that will be troublesome.
/** | ||
* @ngdoc directive | ||
* @name patternfly.filters.component:pfFilterResults | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@restrict here, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok addressing $scope in another PR
16bb57a
to
f90a590
Compare
This PR also downgrades angular bootstrap version to 2.2.x due to issue angular-ui/bootstrap#6364