-
Notifications
You must be signed in to change notification settings - Fork 577
Add PayPal App Switch Overlay #2484
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
Conversation
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.
looks good apart from the one comment 👍
@@ -501,6 +501,15 @@ export type ButtonExtensions = {| | |||
resume: () => void, | |||
|}; | |||
|
|||
type ShowPayPalAppSwitchOverlay = {| |
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.
we also need to accept the universal link to render the continue link to app.
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.
I've added that logic in the focus function in setupOverlays
I can update the focus function to take the args props.redirect
& url.href
and call props.redirect(url.href)
from inside the overlay component
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.
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.
@ravishekhar , I attempted a solution that took the url as an argument and ran into issues running the karma tests with the redirect call now being a part of overlay.jsx
With the Karma framework, there is no way to mock location.href
or location.redirect
. When the redirect is called, the karma test runner is interrupted and the tests fail.
} | ||
} catch { | ||
// an error will be thrown if the overlay is not found, which means overlay was removed successfully | ||
done(); |
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.
Why is this line have a done() inside the catch, while line 188 doesn't?
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.
getElementRecursive(".paypal-checkout-sandbox")
will return an error if it does not find what it is looking for. For this test, I am expecting no overlay to be found so I want the done condition to be called in the catch statement.
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.
Based on offline discussions we will be taking up the changes to refactor the postMessage passing between SDK and Buttons as a followup PR.
Description
We need a new overlay for the new flow PayPal App Switch
Why are we making these changes? Include references to any related Jira tasks or GitHub Issues
https://paypal.atlassian.net/browse/DTPPCPSDK-2897
Reproduction Steps (if applicable)
Screenshots (if applicable)
PayPal.App.Switch.Overlay.mov
Dependent Changes (if applicable)
Groups who should review (if applicable)
❤️ Thank you!