This fixes #61 and fixes #72 #73
Conversation
|
I personally think it's better to do one PR per issue. Also if you use "Fixes #" in your commit it will close the issue automatically on merge. See closing issues via commit messages. |
| @@ -105,7 +105,7 @@ describe('Directive: pfSparklineChart', function() { | |||
|
|
|||
| expect(isolateScope.config.data.x).toBe("dates"); | |||
| expect(isolateScope.config.data.columns.length).toBe(2); | |||
| expect(isolateScope.config.data.columns[0][1]).toBe($scope.data.xData[1]); | |||
| expect(isolateScope.config.data.columns[0][1].toString()).toBe($scope.data.xData[1].toString()); | |||
There was a problem hiding this comment.
Why is the toString() necessary after these changes?
There was a problem hiding this comment.
I believe the unit test was checking that the actual javascript objects were the same, after the change:
'+' scope.config = angular.merge({}, scope.defaultConfig, scope.config);
'-' scope.config = $.extend(true, angular.copy(scope.defaultConfig), scope.config);
They don't appear to be, even though their contents are the same. To me the angular.merge and the $.extend cmds should result in new javascript objects, so I'm not sure why/what is different between the commands. In other words, I wouldn't think the 'toString()' would be neccesary.
There was a problem hiding this comment.
Thank makes sense, thanks for the explanation.
Disregard this, I didn't realize the issues were so closely related. |
|
Also, the commit summary itself should be under 72 characters with extra info in the extended description, if necessary. Otherwise, LGTM. Ok, thanks, go to know
|
|
LGTM but I would rebase and update the commit message as Walden suggested. Ok, fixed commit messages
|
|
@dtaylor113 I don't think "addresses issue..." will close it. See the list of acceptable keywords for closing issues. |
|
LGTM |
❓ ❗ |
|
Didn't realize the check was still pending... |
Addresses Issues
#61 "Missing info. in Sparkline chart ng-docs"
#72 "If jQuery is not being used in application , than pfDonutPctChart directive having issue with $.extend function"