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

Bugfix/ad 55 onsubmit incorrect handling #38

Merged
merged 10 commits into from
Nov 17, 2021

Conversation

er1z
Copy link
Contributor

@er1z er1z commented Nov 16, 2021

No description provided.

.adyen-submit.hidden
.adyen-submit.hidden,
.dropin-container.hidden,
.adyen-method-grid.hidden
{
display: none;
}

Choose a reason for hiding this comment

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

missing new line at the end, wrong code format in almost whole file.
use this https://www.freeformatter.com/css-beautifier.html
to format css

Copy link
Contributor Author

Choose a reason for hiding this comment

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

phpstorm re-format will be sufficient?

@@ -34,14 +34,14 @@ document.addEventListener('DOMContentLoaded', (e) => {

Choose a reason for hiding this comment

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

Rename the variables (remove the dollar sign, e.g. '$') - this is typical for jquery, and you wrote the vanilla js code, don't confuse the reader! : D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional, we've discussed via DM.

@@ -34,14 +34,14 @@ document.addEventListener('DOMContentLoaded', (e) => {

showSubmitButton(false);

adyenMethod.querySelector('.adyen-method-grid, .dropin-container').style.display = '';
adyenMethod.querySelector('.adyen-method-grid, .dropin-container').classList.remove('hidden');

Choose a reason for hiding this comment

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

this works but it i dont think tis correct.

here querySelector will find First element with matching class - the rest will be missed - i dont know what you want to do here, but if you want just 1 specific element, you shouldnt need 2 classes to find it.

If you want ALL elements with these classes, you should use querySelectorAll, and then in array.forEach((item)=>{
item.classList.remove('hidden')
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's intentional — the same code is executed in two different contexts; first .adyen-method-grid, second .dropin-container, it won't happen for two at once.

@@ -34,14 +34,14 @@ document.addEventListener('DOMContentLoaded', (e) => {

Choose a reason for hiding this comment

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

Line 19: showSubmitButton - schould be toggleSubmitButton or handleSubmitButton - cuz its toggling it's visibility, not just showing :D

@@ -10,6 +10,7 @@

let checkout = null;
let configuration = {};
let $form = $container.closest('form');

Choose a reason for hiding this comment

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

change variable name (dolar sign thing as above)

@@ -76,6 +101,8 @@
};

const init = () => {

Choose a reason for hiding this comment

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

Inside init function you can add if statements, to check if its possible to run functions, and return nothing/error if not - you will not initialize adyen then - but idk if its smthing you need

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you recommend?

@er1z er1z merged commit 62f3466 into master Nov 17, 2021
@er1z er1z deleted the bugfix/AD-55-onsubmit-incorrect-handling branch November 17, 2021 08:49
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