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.

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

feat: add optional errorCallback #314

Merged
merged 2 commits into from
Sep 16, 2020
Merged

Conversation

bovandersteene
Copy link
Contributor

With google or other providers you can get following error when you are browsing in private modus:
error: "idpiframe_initialization_failed", details: "Cookies are not enabled in current environment."

In this case I want to inform the user instead of having a silence error. My proposal is to add an erroCallback, where you can add an alert or something else to avoid that the user is not informed.

With google or other providers you can get following error when you are browsing in private modus:
````error: "idpiframe_initialization_failed", details: "Cookies are not enabled in current environment."```

In this case I want to inform the user instead of having a silence error. My proposal is to add an erroCallback, where you can add an alert or something else to avoid that the user is not informed.
Copy link
Contributor

@jaibatrik jaibatrik left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The changes look good to me.

I feel the name errorCallback a bit generic though; but at the same time, I cannot think of anything better as well.

@bovandersteene
Copy link
Contributor Author

@jaibatrik If you find a better name, let me know. But I have no idea neither

@jaibatrik
Copy link
Contributor

I think, we can name it onError for now. Thanks!

@bovandersteene
Copy link
Contributor Author

@jaibatrik I've renamed it, so it will be ready to merge I guess?

@jaibatrik jaibatrik merged commit 2179f03 into abacritt:master Sep 16, 2020
@jaibatrik
Copy link
Contributor

Thanks, I'll push this to npm in a few days.

@jaibatrik
Copy link
Contributor

Holding on for the refactor related to #280.

@jaibatrik
Copy link
Contributor

Pushed this to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants