-
Notifications
You must be signed in to change notification settings - Fork 14
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
Update Checkout block UI #117
Conversation
5a516ac
to
9db4630
Compare
@@ -51,7 +49,7 @@ const KomojuPaymentModule = (() => { | |||
}; | |||
} | |||
|
|||
function submitFields(fields) { | |||
async function submitFields(fields) { |
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.
Great catch!
I just thought we need to carefully test both successful payments and failing payments, because making this function really async
could change the timing this function returns, and thus could result in another timing issues. It's not blocking this PR but I expect you would double-check the behaviour manually. 🙏
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.
Thank you for the check!
I've updated the submitFields function based on the feedback. I removed the unnecessary async keyword from the Promise constructor to clarify the function's control flow and error handling, which should help avoid any potential timing issues.
}) | ||
: null; | ||
const description = window.wp.htmlEntities.decodeEntities(settings.description || window.wp.i18n.__('title', 'komoju_woocommerce')); | ||
const descriptionDiv = createElement('div', |
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.
Could we set descriptionDiv
to null
if description
is falsy (i.e., zero-length string), maybe?
AIUI description
can be empty, notably due to the default configurations for plugin versions <= 3.0.9. In such cases descriptoinDiv
will be an empty <div>
, which just adds a void blank line under the payment name - I fear that doesn't look good.
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.
I set descriptionDiv
to null when it is empty.
I also changed the server-side behaviour how it sets the description. Previously, it was setting default description when the description is empty. Now user are able to empty description.
Since description div & komojuField can be null, null checks have been added in the code too.
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.
Generally LGTM, just left few opinionated comments. Feel free to ping me for a follow-up review!
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.
LGTM! Thanks for this great work!
Changes