Skip to content
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.

Sign up for GitHub

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

Jump to bottom

docs: added getResponse instance method #188

Merged
merged 1 commit into from Dec 28, 2019
Merged

docs: added getResponse instance method #188

merged 1 commit into from Dec 28, 2019

Conversation

AndyOGo
Copy link
Contributor

@AndyOGo AndyOGo commented Oct 30, 2019 •
edited

fixes #185

Changes:

  • add getResponse instance method to readme already done

  • adds getResponse example which just alerts it

@coveralls
Copy link

coveralls commented Oct 30, 2019 •
edited

Pull Request Test Coverage Report for Build 945

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 944: 0.0%
Covered Lines: 79
Relevant Lines: 79
💛 - Coveralls

AndyOGo
AndyOGo commented Oct 30, 2019
View reviewed changes
example/src/modules/example.js
Comment on lines +83 to +85
this.captcha.getResponse().then(response => {
alert(response);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarneeh
The google getResponse API will return just an "" empty string until the challenge is resolved.

I'm just curious if it should be treated as a Promise rejection 🤔
Current implementation would only reject the Promise if the underlying Google API would throw, which I consider valid behaviour too.

@jsardev
Copy link
Owner

jsardev commented Oct 30, 2019

Hey @AndyOGo, I guess you didn't notice, that I've already created a PR for this: #187 :) We can keep both tho: we'll just keep the example from this PR.

@AndyOGo
Copy link
Contributor Author

AndyOGo commented Oct 30, 2019

@sarneeh Thanks for your quick answer.
Sorry I didn't check for open PR's.
Feel free to cherry pick or close this one

@AndyOGo
Copy link
Contributor Author

AndyOGo commented Oct 30, 2019 •
edited

Hi @sarneeh I saw you merged #187, so I pulled down your latest master and rebased this one interactively :)

@AndyOGo AndyOGo force-pushed the feat/add-docs-for-get-response branch from 1875538 to 51e75f5 Compare October 30, 2019 17:22
@AndyOGo
Copy link
Contributor Author

AndyOGo commented Dec 10, 2019

@sarneeh
Do you have any time soon, to check if you agree with this example?

@jsardev jsardev merged commit 274e4cf into jsardev:master Dec 28, 2019
@AndyOGo AndyOGo deleted the feat/add-docs-for-get-response branch December 28, 2019 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsardev jsardev jsardev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add info about getResponse to docs
3 participants
@AndyOGo @coveralls @jsardev