Improving the handling of express method titles #3844
Conversation
…o dev/improving-handling-of-ece-method-titles
| */ | ||
| public static function get_payment_method_titles( $include_link = false ) { | ||
| $titles = [ | ||
| 'apple_pay' => WC_Stripe_Express_Payment_Titles::APPLE_PAY, |
There was a problem hiding this comment.
Another implementation option would be translated names, like:
'apple_pay' => __( 'Apple Pay', 'woocommerce-gateway-stripe' ),But not sure if it is worth since those are brands, which I think are immutable.
…/github.com/woocommerce/woocommerce-gateway-stripe into dev/improving-handling-of-ece-method-titles
There was a problem hiding this comment.
Hi @wjrosa
Thanks for the effort in this PR. After reviewing this, I think that the changes might not add significant value in improving the codebase. It is a bit overengineering to have a new class for the express method titles and also reduces the readability of the code.
We have implemented constants for the payment methods in #3449. I think that was a good refactor and we should use that constant class to store the express method titles as well instead of making it complex with more new classes.
It would be better to hold off on merging this one. Let me know what you think and if you have a strong opinion in favor of these changes 👀
| $order->save(); | ||
| } elseif ( 'payment_request_api' === $payment_request_type ) { | ||
| $order->set_payment_method_title( 'Payment Request (Stripe)' ); | ||
| $payment_method_title = ''; |
There was a problem hiding this comment.
Just making it easier to read. And fixing the suffix part (it is dynamic).
| @@ -41,7 +41,11 @@ public function __construct() { | |||
| * @return string | |||
| */ | |||
| public function get_title( $payment_details = false ) { | |||
| $wallet_type = WC_Stripe_Payment_Methods::AMAZON_PAY === ( $payment_details->type ?? null ) ? WC_Stripe_Payment_Methods::AMAZON_PAY : ( $payment_details->card->wallet->type ?? null ); | |||
| if ( WC_Stripe_Payment_Methods::AMAZON_PAY === ( $payment_details->type ?? null ) ) { | |||
There was a problem hiding this comment.
Just making it easier to read.
Thanks, Mayisha! Oh OK, I have refactored it now just to follow the current implementation of the payment method labels (using the existing constant instead of adding the new one). I have also fixed all the title suffix calls (which were not checking for the filter value). Let me know what you think of this simpler approach 👀 |
Changes proposed in this Pull Request:
This PR improves how we handle express payment method titles by introducing new constants and methods to replace duplicate code.
Testing instructions
Code review should be enough. Check if tests are still passing.
changelog.txtandreadme.txt(or does not apply)Post merge