Navigation Menu

Skip to content
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

Move inline settings javascript to ppec-settings.js #676

Merged
merged 1 commit into from Feb 10, 2020

Conversation

jorgeatorres
Copy link
Member

Description

When the PPEC gateway is initialized it loads file includes/settings/settings-ppec.php, which returns the set of configuration settings for the gateway.
This file also enqueues some inline javascript in order to handle the hiding and showing of certain settings on the settings screen (for example: to hide Live credential fields when the "Sandbox" environment is selected, and viceversa).

Because it is unconditionally enqueued inline, this JS gets added to WC pages on the frontend (and even to all pages if using things like the mini-cart widget). Because the CSS IDs targeted by the code are very specific, this is probably fine most of the time, but it's unnecessary nevertheless. We also have an admin-side settings JS file (ppec-settings.js) which is not only not loaded on the frontend but also seems like a more appropriate location for this code.

This PR moves the inline JS to ppec-settings.js.

Note: I couldn't find a reason for this JS to be enqueued on the frontend, but I might've missed something. If that's the case, feel free to comment and/or close the PR.

Steps to test

  1. Checkout master or 1.7_dev.
  2. Navigate to a product page, or to any page (if using the Storefront theme).
  3. View the source code for the page and confirm that there's an inline script for the PPEC admin settings page, which looks something like this (click for full code):
    jQuery( function( $ ) {
            var ppec_mark_fields      = '#woocommerce_ppec_paypal_title, #woocommerce_ppec_paypal_description';
            var ppec_live_fields      = '#woocommerce_ppec_paypal_api_username, #woocommerce_ppec_paypal_api_password, #woocommerce_ppec_paypal_api_signature, #woocommerce_ppec_paypal_api_certificate, #woocommerce_ppec_paypal_api_subject';
    [...]
  4. Checkout no_inline_settings_js.
  5. Confirm that there's no inline JS for the admin settings.
  6. Go to the plugin settings on the backend (WC > Settings > Payments > PayPal Checkout > Manage) and confirm that the settings page works correctly. For example:
    • Verify that toggling the "Environment" shows/hides the Live/Sandbox fields accordingly.
    • Confirm that checking/unchecking cart/mini-cart/single product/checkout specific settings shows and/or hides the dependent fields.

@mattallan
Copy link
Contributor

Thanks for this PR @jorgeatorres! I was also scratching my head when I saw this JS being enqueued like this.

I couldn't find a reason for this JS to be enqueued on the frontend, but I might've missed something. If that's the case, feel free to comment and/or close the PR.

I can't see any reason for this either. I also didn't realise this was being loaded on the frontend (product pages etc) and I'm going to assume that wasn't intended.

I've tested this PR by making sure the scripts are no longer loaded on the frontend pages and the settings page is working as intended. I also double checked all the JS being moved is only related to the settings page - LGTM


Note: I've rebased this PR to include some extra JS which was merged into 1.7_dev but then went missing. These lines specifically: f3fcfd6

Merging :)

@mattallan mattallan merged commit b0be1b5 into 1.7_dev Feb 10, 2020
@mattallan mattallan deleted the no_inline_settings_js branch February 10, 2020 03:21
@mattallan mattallan restored the no_inline_settings_js branch April 17, 2020 00:47
mattallan pushed a commit that referenced this pull request Apr 17, 2020
Move inline settings javascript to `ppec-settings.js`
mattallan pushed a commit that referenced this pull request May 19, 2020
Move inline settings javascript to `ppec-settings.js`
mattallan pushed a commit that referenced this pull request May 19, 2020
Move inline settings javascript to `ppec-settings.js`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants