Skip to content

Conversation

bc-fetisov
Copy link
Contributor

@bc-fetisov bc-fetisov commented Oct 2, 2019

…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

Screen Shot 2019-10-18 at 14 27 22
Screen Shot 2019-10-18 at 14 28 36

Screen Shot 2019-10-18 at 14 28 21

@bigcommerce/frontend

@bc-fetisov bc-fetisov force-pushed the PAYPAL-7 branch 2 times, most recently from 82ccd39 to 3e52b93 Compare October 2, 2019 16:03
@@ -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> {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @davidchin

@BC-ACherednichenko BC-ACherednichenko changed the title WIP [do not merge] feat(payments): PAYPAL-7 Pass in merchant ID on Pa… feat(payments): PAYPAL-7 Pass in merchant ID on Pa… Oct 18, 2019
@BC-ACherednichenko
Copy link
Contributor

ping @davidchin

@@ -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;
Copy link
Contributor

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 || {};

const scriptAttributes = options && options.attributes;

for (const key in scriptAttributes) {
if (scriptAttributes.hasOwnProperty(key)) {
Copy link
Contributor

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]);
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@davidchin davidchin left a 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 👍

@BC-ACherednichenko BC-ACherednichenko merged commit 890c8ad into bigcommerce:master Oct 23, 2019
@bc-fetisov bc-fetisov deleted the PAYPAL-7 branch November 12, 2019 14:31
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.

3 participants