Skip to content

Express checkout #10

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Express checkout #10

wants to merge 9 commits into from

Conversation

rmarinleal
Copy link
Contributor

No description provided.

@rmarinleal rmarinleal requested review from pgarcess and tmeliotpg July 21, 2021 10:12
var initAfterPayExpress = function () {
var spinner = null;
var button = document.getElementById('afterpay_express_button');
if (button.disabled == true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is generally safer.

Suggested change
if (button.disabled == true) {
if (button && button.disabled == true) {

Comment on lines 148 to 164
document.addEventListener('readystatechange', event => {
if (event.target.readyState === "interactive" &&
typeof window.AfterPay !== 'undefined' &&
typeof AfterPay.initializeForPopup != 'undefined'
) {
initAfterPayExpress();
}

// Double check
if (event.target.readyState === "complete" &&
typeof window.AfterPay !== 'undefined' &&
typeof AfterPay.initializeForPopup != 'undefined' &&
document.getElementById('afterpay_express_button').disabled == true
) {
initAfterPayExpress();
}
});
Copy link
Contributor

@tmeliotpg tmeliotpg Jul 21, 2021

Choose a reason for hiding this comment

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

Just some suggestions.
This was done by Scott, he is a full stack eng and excels at front-end.

The button is disabled for a few seconds because the call stack has to loop through all the ready states when the real requirement is the button element and the Afterpay object. Therefore, we could possibly remove further checks and if we need to do so we can start checking from events DOMContentLoaded or load

I think passing jQuery as a param in an IIFE like Scott did is safer and allows avoiding having to loop through the documents ready states.

Additionally, the Ajax methods depend on jQuery therefore an exception would be thrown if jQuery is not in the window object.

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.

2 participants