Skip to content
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

Updating ach example #218

Merged
merged 5 commits into from
Oct 18, 2023
Merged

Updating ach example #218

merged 5 commits into from
Oct 18, 2023

Conversation

abhipillai
Copy link
Contributor

@abhipillai abhipillai commented Oct 18, 2023

Please explain the changes you made here.
Updates ACH example and removes the card element.

Does this close any currently open issues?

@abhipillai abhipillai marked this pull request as ready for review October 18, 2023 16:17
@abhipillai abhipillai requested a review from a team as a code owner October 18, 2023 16:17
console.error('Initializing Card failed', e);
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for removing the card flow from this example. It just muddles up the example as is.

displayPaymentResults('SUCCESS');

console.debug('Payment Success', paymentResults);
tokenize(paymentMethod, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know ACH relies mostly on the event listener, but I feel like this should still be awaited..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the errors will now be captured via the listener so we don't need to await this anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, the async keyword should be removed from the tokenize() method definition on line 50.

const achOptions = getACHOptions(paymentForm);
await handlePaymentMethodSubmission(event, ach, achOptions);
});
// Checkpoint 2.
Copy link
Contributor

Choose a reason for hiding this comment

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

This checkpoint is directly at odds with the idea of this example being completely focused on ACH and ACH alone, I think checkpoint 1 was the card example. We should pick a lane here - each payment method is independent, or the examples build on each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good callout

@abhipillai abhipillai merged commit 0438dd7 into main Oct 18, 2023
1 check passed
@abhipillai abhipillai deleted the updating-ach-example branch October 18, 2023 16:54
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