-
Notifications
You must be signed in to change notification settings - Fork 8
feat(payments): PAYPAL-7 Pass in merchant ID on Pa… #9
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
82ccd39
to
3e52b93
Compare
…r PayPal Express Checkout
…r PayPal Express Checkout
src/script-loader.ts
Outdated
@@ -22,12 +26,18 @@ export default class ScriptLoader { | |||
private _requestSender: RequestSender | |||
) {} | |||
|
|||
loadScript(src: string, options?: LoadScriptOptions): Promise<void> { | |||
loadScript(src: string, options?: LoadScriptOptions, scriptAttributes?: ScriptAttributes): Promise<void> { |
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.
Just add the additional attributes as a part of LoadScriptOptions
instead of introducing a third parameter.
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.
Done @davidchin
…r PayPal Express Checkout - CR fixes
ping @davidchin |
src/script-loader.ts
Outdated
@@ -27,6 +32,13 @@ export default class ScriptLoader { | |||
this._scripts[src] = new Promise((resolve, reject) => { | |||
const script = document.createElement('script') as LegacyHTMLScriptElement; | |||
const { async = false } = options || {}; | |||
const scriptAttributes = options && options.attributes; |
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.
You can combine this with the line above. i.e.:
const { async = false, attributes = {} } = options || {};
src/script-loader.ts
Outdated
const scriptAttributes = options && options.attributes; | ||
|
||
for (const key in scriptAttributes) { | ||
if (scriptAttributes.hasOwnProperty(key)) { |
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.
You can just use Object.keys
so you don't need to do this check. i.e.:
Object.keys(attributes)
.forEach(key => {
script.setAttribute(key, attributes[key]);
});
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.
Fixed
…r PayPal Express Checkout - CR fixes
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.
Cool, thank you LGTM 👍
…yPal button script for PayPal Express Checkout
What?
Add possibility to add script data attributes
Why?
The PayPal Shopping feature should know which merchants to target for PayPal Store Cash
Testing / Proof
@bigcommerce/frontend