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

[SDK-1395] Refactor loginWithPopup to optionally accept an existing popup window #368

Merged
merged 4 commits into from
Mar 12, 2020

Conversation

stevehobbsdev
Copy link
Contributor

@stevehobbsdev stevehobbsdev commented Mar 2, 2020

Description

This PR adds a new option to loginWithPopup that would allow the developer to pass an existing popup window to be used instead of the SDK creating its own. This allows the popup to work better in scenarios like on iOS where the popup should be opened inside a button click handler without any async wrappers.

It also allows us to fix an issue where the popup sometimes doesn't work properly inside a PWA. The existing logic creates the pop window and then sets the URL, whereas now in this PR those two steps are unified.

References

Closes #300

Testing

Manually tested by adding a new button to the playground that allows a popup window to be specified when calling loginWithPopup. Unit tests have also been added.

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@stevehobbsdev stevehobbsdev added bug This points to a verified bug in the code size: small CH: Added PR is adding feature or functionality labels Mar 2, 2020
@stevehobbsdev stevehobbsdev added this to the vNext milestone Mar 2, 2020
@stevehobbsdev stevehobbsdev requested a review from a team March 2, 2020 16:30
@zewa666
Copy link

zewa666 commented Mar 3, 2020

worked for me on Android, had no opportunity to test on iOS due to lack of device. But since @charsleysa mentioned doing it this way in his patch I would guess it should work as well.

@stevehobbsdev
Copy link
Contributor Author

@zewa666 Thanks for looking. Can you confirm that it solves your original problem of opening the popup window without the need for a setTimeout call?

@zewa666
Copy link

zewa666 commented Mar 3, 2020

@stevehobbsdev sorry for being unclear. Yes I've reverted my changes, checked out your branch in my cloned repo, ran npm run build and tried it on the phone. It did pass for me

@stevehobbsdev
Copy link
Contributor Author

Thanks @zewa666 !

Copy link

@charsleysa charsleysa left a comment

Choose a reason for hiding this comment

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

LGTM

@stevehobbsdev stevehobbsdev merged commit 86720a6 into master Mar 12, 2020
@stevehobbsdev stevehobbsdev deleted the issue/300 branch March 12, 2020 09:29
ygist added a commit to thrivehealth/auth0-react-1 that referenced this pull request Oct 26, 2020
for use with iOS to open a popup, similar to loginWithPopup.

refer to auth0/auth0-spa-js#368 for additional details
adamjmcgrath pushed a commit to auth0/auth0-react that referenced this pull request Oct 27, 2020
for use with iOS to open a popup, similar to loginWithPopup.

refer to auth0/auth0-spa-js#368 for additional details
hrrytech92 added a commit to hrrytech92/auth0-react that referenced this pull request May 31, 2022
for use with iOS to open a popup, similar to loginWithPopup.

refer to auth0/auth0-spa-js#368 for additional details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This points to a verified bug in the code CH: Added PR is adding feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PWA loginWithPopup blocked
4 participants