-
Notifications
You must be signed in to change notification settings - Fork 558
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: adds button message popup handling for purchase protection #2376
base: main
Are you sure you want to change the base?
Conversation
…/github.com/paypal/paypal-checkout-components into feature/DTCRCMERC-2925-purchase-protect-msg
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 don't see any issues with this but wanted to ask a question about webviews
is this completely configurable by the merchant? One issue we see with window.open
/ popups is that they don't work on mobile in-app webviews. Our checkout popup actually renders inline on the page inside of an iframe if it detects that it is in a webview.
not sure if we need to go that far for this popup or if we suggest to merchants to not use that setting if they are using mobile in-app webviews
return window.open(uri, "_blank"); | ||
} | ||
} catch (err) { | ||
getLogger() |
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 would suggest adding a fallback logic here to fallback to modal experience if popup fails.
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.
Thanks for the suggestion @ravishekhar. Sounds like we will be doing some dev work to make a purchase protection modal to address this issue that you and @wsbrunson called out.
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.
Quick find but otherwise lgtm 😄 👾
@@ -451,14 +452,18 @@ describe(`paypal button message`, () => { | |||
}); | |||
}); | |||
|
|||
describe("modal", () => { | |||
describe.only("modal", () => { |
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.
Was this .only
left in accidentally? 😅
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.
😅 indeed, good eye. let me fix that
Changed back to a draft PR for the time being while the details are worked out internally to address the issue raised about webview popups. |
Description
Adds feature to have button messages open a popup (or new tab, depending on settings) on click instead of always a modal.
Why are we making these changes? Include references to any related Jira tasks or GitHub Issues
Supporting Purchase Protection messages, which will open a popup in cases where showing credit product presentment is inappropriate.
Reproduction Steps (if applicable)
Test on this demo page
Screenshots (if applicable)
Dependent Changes (if applicable)
Groups who should review (if applicable)
❤️ Thank you!