Add connect services WC settings pages #12
Conversation
|
Isn't this super premature since we're only working on shipping services right now? |
I suppose it is - I just keep thinking "no one is going to update" and try to cram it all in now. |
That's a fair enough point. |
| $methods[] = new WC_Connect_Shipping_Method( $value ); | ||
| } | ||
|
|
||
| return $methods; | ||
| } | ||
|
|
||
| public function woocommerce_payment_gateways( $gateways ) { |
There was a problem hiding this comment.
I think this (and the shipping method) could be simplified/easier to follow a bit:
public function woocommerce_payment_gateways( $gateways ) {
$wcc_gateways = (array) $this->services[ 'checkout' ];
if ( empty( $wcc_gateways ) ) {
return $gateways;
}
require_once( plugin_basename( 'classes/class-wc-connect-payment-gateway.php' ) );
return array_merge( $gateways, array_map( array( $this, 'instantiate_payment_gateway' ), $wcc_gateways ) );
}
public function instantiate_payment_gateway( $gateway ) {
return new WC_Connect_Payment_Gateway( $value );
}A few things I like about this better:
- Not creating as many obscure variables
- Better named variables
- Clearer to follow what's going on
- Early returns are best 👍
- 2 smaller methods/better abstraction
Sorry for being so nit-picky.
Thoughts?
There was a problem hiding this comment.
I think this is pretty nit-picky, though you are correct - my variable names are weak and I could return earlier.
I'm not a huge fan of introducing an additional function and an array map - it's not as quickly readable for something so simple.
I'll rename some variables and return earlier, then let's review.
There was a problem hiding this comment.
wfm, i am definitely pushing it here, so a happy compromise will be good.
jeffstieler
force-pushed
the
add/connect-services-wc-settings-pages
branch
from
0457b04 to
91a4f30
Compare
| @@ -49,23 +49,55 @@ public function __construct() { | |||
| 'method_title' => __( 'USPS (WooCommerce Connect)', 'woocommerce' ), | |||
| 'method_description' => __( 'Shipping via USPS, Powered by WooCommerce Connect', 'woocommerce' ) | |||
| ), | |||
| ) | |||
| ), | |||
| 'checkout' => array( | |||
There was a problem hiding this comment.
Instead of checkout how about payment ?
There was a problem hiding this comment.
I'm fine with that - was just mapping to the tab name in WC settings.
|
All feedback addressed - just need the go ahead on a merge. |
…nd payment gateway filter methods.
jeffstieler
force-pushed
the
add/connect-services-wc-settings-pages
branch
from
b11b1b2 to
eed27b2
Compare
|
|
…gs-pages Add connect services WC settings pages
Addresses the first item here: #9
I had originally intended this to have more functionality and be more clever - basically dynamically handling the addition of services that would fall into each "tab" on the WC Settings pages (General, Checkout, Shipping, etc).
It turns out, as far as I can tell, that only Shipping and Checkout get loaded via WC core. Assuming this is correct, I think we don't need to go further at this time.