Depend on Angular stable, use new bower.json naming, fix bug when jQuery is present #4
Conversation
* Without proper annotations injection does not work anymore * Used ngmin (https://github.com/btford/ngmin) to add annotations * Reinserted the comments manually
|
In the meantime I added a new commit that enables this library to be used minified (using standard uglifyJS for instance) |
|
I just forked and committed skivvies/angular-oauth@ffe94596 and was about to submit my own PR before finding this one, which I think should be merged instead. |
|
@Enginous can we merge this or is there anything wrong with this PR ? |
| return { | ||
| // TODO: get/set might want to support expiration to reauthenticate | ||
| // TODO: check for localStorage support and otherwise perhaps use other methods of storing data (e.g. cookie) | ||
| // TODO: check for localStorage support and otherwise perhaps use other methods of s |
There was a problem hiding this comment.
I believe you have accidentally deleted end of the sentence.
UPDATE: And in many other places below.
|
I'm terribly sorry about the delay. Will review this evening. |
| resolved = false; | ||
|
|
||
| var formatPopupOptions = function(options) { | ||
| var deferred = $q.defer(), params = angular.extend(getParams(), extraParams), url = config.authorizationEndpoint + '?' + objectToQueryString(params), resolved = false; |
There was a problem hiding this comment.
I actually don't think this is more readable than it was before, and I try to stick loosely to an 80-character column limit in the code (as per Google's JS guidelines).
|
@vperron Thanks for the contribution. It looks good to me. I'm ready to merge this if you can bring back the comments that were lost (you can wrap them to 80 cols if you'd like) and fix my one comment about style. |
Hello,
I have made a few changes in the code since I'll be probably using this in one of my projects.
For that project and many others the policy is towards long-term stability and durability; it did seem to me reasonable to depend on a stable version of angular, and the lowest the better...
Also, since bower is slowly deprecating component.json, it's better to switch to that; I think it's better not to follow the bad habit of keeping it as 'component.json', it just slows down the advance of bower.
Finally, I found a quite nasty bug when used with some versions of jQuery; jQuery does redefine the Event object, in those cases the properties we are lokking for (the token for instance) are no more in event.data but in event.originalEvent.data.
Of course the change takes care of both cases. Tell me what you think !
Best regards,