Add pfSimpleSort directive with unit tests and ng-docs #76
Conversation
| scope: { | ||
| config: '=' | ||
| }, | ||
| transclude: false, |
There was a problem hiding this comment.
Yes. I typically set it anyhow. I will remove though.
|
It would be nice if I could use the arrow keys and enter to select a filter. This would be a good opportunity to incorporate the |
| </example> | ||
| */ | ||
| angular.module('patternfly.sort').directive('pfSimpleSort', | ||
| function ($document) { |
There was a problem hiding this comment.
$document doesn't appear to be used anywhere.
|
LGTM |
|
Addressed the open issues. Let me know and I will squash before merge. |
| if ($scope.config.isAscending) { | ||
| iconClass = 'fa fa-sort-alpha-asc'; | ||
| } else { | ||
| iconClass = 'fa fa-sort-alpha-desc'; |
There was a problem hiding this comment.
What about using ng-class for these classes rather than having them in JS code? Sorry my comment wasn't specific enough before.
There was a problem hiding this comment.
There are 4 variations all which would check almost the same things. I thought it clearer to provide a function for this logic.
There was a problem hiding this comment.
That's a good point and this way we can test it. Speaking of, I don't see any tests for this function.
There was a problem hiding this comment.
The tests check for the correct icon class after selecting different sort fields and directions.
There was a problem hiding this comment.
But we don't want to unit test this explicitly? I expected a test for it since it's on the $scope and thus part of the API.
jeff-phillips-18
force-pushed
the
master
branch
from
2e37525 to
4038885
Compare
|
Issue added for i18n support for icons in the direction button. |
|
LGTM |
|
|
||
| .simple-sort .sort-direction { | ||
| font-size: 22px; | ||
| color: #222222; |
There was a problem hiding this comment.
in the future just use #222 if everything is repeating.
jeff-phillips-18
force-pushed
the
master
branch
from
4038885 to
16f76f6
Compare
Add pfSimpleSort directive with unit tests and ng-docs
No description provided.