-
-
Notifications
You must be signed in to change notification settings - Fork 76
Support 18n for Braintree error messages #133
Support 18n for Braintree error messages #133
Conversation
It seems the test fails for a timeout error or something else unrelated. |
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.
Thanks! Much better than the current implementation. 👍
@@ -6,14 +6,11 @@ SolidusPaypalBraintree = { | |||
clientTokens: Spree.pathFor('solidus_paypal_braintree/client_token'), | |||
transactions: Spree.pathFor('solidus_paypal_braintree/transactions') | |||
}, | |||
|
|||
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.
unnecessary whitespace sneaked into here :)
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.
We need to change the js.erb
into a html.erb
file and render this in the payment form. Otherwise translation changes won't be picked up by next asset compilation.
@@ -0,0 +1,17 @@ | |||
BraintreeError = { |
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.
Sorry, just saw that this is a js.erb
file instead of an html partial. Although not very obviously, but js.erb
files are very problematic, because changes to the translations won't trigger this file to be re-compiled on next asset compilation. Please make this is html.erb file that gets rendered inside the payment form.
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 feedback.
I changed this behavior and now it should work fine.
By using an html.erb partial we make sure that if you change the locale files you don’t need a new asset precompile to actually pick up the changes
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. This is great
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.
@Gamabunta94 this seems great, thanks!
@tvdeyen can we merge this one? I think it's ok but I don't have permissions to merge here. |
@kennyadsl merged |
Thanks! |
With this PR I want to add the localization for Braintree error messages.
For this purpose I add the Braintree error messages in the localization rails file and show it with the corresponding slug returned from the Braintree response.
I also added Italian translation of the messages.