-
Notifications
You must be signed in to change notification settings - Fork 26
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
Example and doc fixups #35
Conversation
This involves updates to work with the new-model stripped-down 'request' Ruby objects, which don't have an embedded appId or challenges of their own. (And also updates to the example's own Gemfile, of course.)
This requires supplying appId and challenge values to JS separately from the request objects (which no longer have these items bundled), and deleting 'as_json' method invocations which appear to no longer be necessary.
Thanks @rst for taking care of this. Looks good to me. Any comments from you side @mastahyeti ? |
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.
One change, but otherwise this looks good to me.
README.md
Outdated
var signRequests = <%= @sign_requests.as_json.to_json.html_safe %>; | ||
var signRequests = <%= @sign_requests.to_json.html_safe %>; | ||
var challenge = #{@challenge.to_json.html_safe}; | ||
var appId = #{@app_id.to_json.html_safe}; |
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.
Should these two lines use ERB tags instead of #{}
?
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.
Corrected. (I'd pasted those lines in from the HAML in the example, which, as before, uses #{...} syntax consistently.)
Corrects some usages of #{...} syntax taken from HAML in the example code.
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.
👍
This PR updates the example to work with the JS and Ruby APIs provided by version 1.0.0 of the gem. (This requires adjustments as the request objects no longer have separate appIds or challenges.) This resolves issues #31, #32, and provides examples which serve as better models for what the reporter was trying to do in #34.
It also updates the Ruby and JS examples in the README to match the current codebase, and adds a note that in actual usage, registration objects would be associated with a particular user on creation, and subsequent queries for them would ordinarily be scoped to those associated with the particular user trying to authenticate.