Make Mini-cart settings independent from cart #775
Conversation
Changes: - Changed section title "Button Settings" to "Global Button Settings" to set up the section as default settings - Added section title for Cart page settings - Added Cart page specific configuration options - Added option to enable/disable PPEC on Mini-cart (enabled by default)
Changes: - Modified function maybe_enqueue_checkout_js() to load mini-cart specific settings irrespective of cart settings - Modified function enqueue_scripts() to load cart specific settings instead of default
Changes: - Added collapse/expand for Cart specific settings - Modified collapse/expand for Mini-cart specific settings - Added comments to identify section specific JS
achyuthajoy
added
priority: low
achyuthajoy
added
the
type: question
There was a problem hiding this comment.
Hi @achyuthajoy!
Thanks for submitting this 🙇! I've left some feedback, which mostly deals with minor things. You did an awesome job capturing the requirement and proposing a solution, even though it involved navigating very different parts (settings, JavaScript, PHP) 💯.
Somethings I noticed and that match some of your questions/observations:
- I don't think we should add context-specific configuration for the cart at this point. I know the UI isn't ideal and what you did here works perfectly fine, but that the cart and "default" settings are shared is something that has been that way for a long time… and so we could be adding even more confusion. Also, it's not clear what the defaults mean if you enable context specific settings for all contexts. In contrast, doing context specific configuration for other contexts (while more work) does mean the only context remaining (cart) uses the default, which is the current behavior.
- We could look into clarifying all of this in a different PR, while we concentrate on separating the mini-cart toggle in this one. We can also discuss how to migrate to the new settings, if necessary.
- As you yourself have pointed out, when upgrading things don't work correctly. You see the mini-cart enabled but it really isn't. Instead of adding an admin notice we could look into automatically (and silently) populating the settings accordingly. The value for this new setting (
mini_cart_checkout_enabled) should be the same as forcart_checkout_enabled.
We already do something like this here. You can use that as inspiration, but basically, we would set$_settings['mini_cart_checkout_enabled']to the value of the cart setting if it is not already present in the array. I think this is the right way, but alternatively, you could just leave the "yes" as default and reverse some of the conditional checks so that they do not show the PayPal button in the mini cart when explicitly set to "no".
Once again, good job here 👍. Feel free to respond to the feedback in case you have questions or want to continue the discussion.
| .addBack() | ||
| .toggle( checked ); | ||
| checked && $( '#woocommerce_ppec_paypal_mini_cart_settings_toggle' ).change(); | ||
| if ( ! $('#woocommerce_ppec_paypal_cart_checkout_enabled' ).is( ':checked' ) ) { |
There was a problem hiding this comment.
You're missing a space before the argument to $() here.
| $( '.woocommerce_ppec_paypal_mini_cart' ).closest( 'tr' ).toggle( checked ); | ||
| checked && $( '#woocommerce_ppec_paypal_mini_cart_button_layout' ).change(); | ||
| // Toggle Mini-cart section | ||
| $('#woocommerce_ppec_paypal_mini_cart_checkout_enabled, #woocommerce_ppec_paypal_mini_cart_settings_toggle').change(function (event) { |
There was a problem hiding this comment.
This section here is missing some spaces too. Sadly, our Travis checks don't check Javascript code, but here's the WP coding standards for JavaScript, so that you can take a look: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/.
In terms of spacing, spaces are needed after opening ( and closing ) for function calls, and conditional blocks, etc.
| showHideDefaultButtonSettings(); | ||
| } ).change(); |
There was a problem hiding this comment.
There's no need to remove the space here. See above.
|
|
||
| // Toggle Single Product section |
There was a problem hiding this comment.
Tip 💡: Nothing wrong with this comment, but in general, and unless they add necessary context, we try to avoid adding comments during PR as they can become distracting during code review. Also, comments should end with a dot "." 😸.
| @@ -318,6 +338,7 @@ | |||
| showHideDefaultButtonSettings(); | |||
| } ).change(); | |||
|
|
|||
| // Toggle Checkout Section | |||
| @@ -532,14 +534,15 @@ public function enqueue_scripts() { | |||
| $data = array_merge( $data, $button_settings ); | |||
| } | |||
|
|
|||
| // Load Mini Cart Settings if enabled | |||
| $settings_toggle = 'yes' === $settings->mini_cart_settings_toggle; | ||
| $mini_cart_data = $this->get_button_settings( $settings, $settings_toggle ? 'mini_cart' : '' ); | ||
| foreach ( $mini_cart_data as $key => $value ) { | ||
| unset( $mini_cart_data[ $key ] ); | ||
| $mini_cart_data[ 'mini_cart_' . $key ] = $value; | ||
| } | ||
|
|
There was a problem hiding this comment.
Tip 💡: Similar to my comment about comments, adding or removing lines is something we try to avoid, unless it is to fix compliance with the standards. Added or removed blank lines make for a more complicated changeset or diff view with no real benefit.
| @@ -317,9 +317,9 @@ | |||
| ), | |||
|
|
|||
| 'button_settings' => array( | |||
| 'title' => __( 'Button Settings', 'woocommerce-gateway-paypal-express-checkout' ), | |||
| 'title' => __( 'Global Button Settings', 'woocommerce-gateway-paypal-express-checkout' ), | |||
There was a problem hiding this comment.
I'd suggest we maintain the original wording. I totally understand what you mean here, but we would need to update screenshots and docs and maybe the difference could confuse some users.
| 'type' => 'title', | ||
| 'description' => __( 'Customize the appearance of PayPal Checkout on your site.', 'woocommerce-gateway-paypal-express-checkout' ), | ||
| 'description' => __( 'Customize the default appearance of PayPal Checkout on your site. If page specific customizations are not defined, these will be used by default. <br>For page specific customizations, use the configuration options inside <b>Cart / Mini-cart / Single Product / Regular Checkout</b>', 'woocommerce-gateway-paypal-express-checkout' ), |
There was a problem hiding this comment.
Same as above. For familiarity, maybe we can keep the original description. We could look into making the UI easier in another PR, just so that we can keep this one focused on separating the mini-cart buttons from the cart ones.
| /** | ||
| * Cart / global button settings. | ||
| * Default button settings. |
There was a problem hiding this comment.
Depending on what we decide to do, we might need to go back to the original text here. See my overall review for details.
Description
At the moment, the PPEC on Mini-cart is controlled by the cart option. There are no specific options to disable Mini-cart PPEC and requires PPEC on cart to be enabled to show up.
This PR adds the following changes.
Tested the changes on a new site & existing site that already uses PPEC. Couldn't find any compatibility issues and noticed that the existing settings are carried over.
Testing Changes
Question
Since Mini-cart is enabled based on cart settings, on existing sites when the plugin is updated to include this PR, "Enable PayPal Checkout on Mini-cart" (default field value set to "yes") doesn't take effect if the settings are not saved.
Should we add an admin notice to save the settings once the plugin is updated? Or set the value of "Enable PayPal Checkout on Mini-cart" to "no" by default?
Not sure about documenting the new Global Settings & Mini-cart sections.
Closes #745.