Closed karellm closed 9 years ago
Sorry I haven't had a chance to review the code yet but it sounds like a good idea. I look through the update in the next day or two. Thanks.
You can run the tests with any of the following commands
$ grunt
$ grunt test-amd
$ grunt test-global
I agree the response type should be configurable but all of your other changes break the code and the test no longer pass when I run.
grunt
If you submit two separate pull requests one to make response_type configurable and another for the other changes it would be easier to merge the code. That said I like some of the changes in the second part but not all of them. The setProviderConfig
method seems a bit long and complicated for something that use to be a few lines of code.
Again if you resubmit them as two different pull requests I'll look them over. I suggest just doing response_type
patch from the current master branch as it would be much easier to merge. Also I just need you to submit the changes to the dist/*.js files. Those are there for bower releasees and I can update those when I release a new version.
Thanks for the patch.
Yes, I was expecting this answer. This PR is a mess, I kept working on the branch because I needed to fix the issues quickly. I will make two seperate ones.
Also there are actually a lot more problem then the ones I surfaced here but didn't take the time to tackle them. One of them was the confusion between camelCase and under_score in the query string. This should also be configurable.
The response_type should be configurable. If you look at stripe, they accept a
response_type
ofcode
only. Also you may want to pass more options to the request. So I change the code a bit to parse theproviderConfig
. It generates the request object and set all the values required.I tried running the tests but testem won't run (instructions would be greatly appreciated). It is tough for me to say if it broke anything, but I'm currently testing in an application.