wp-media / imagify-plugin

Speed up your website with lighter images without losing quality.
https://imagify.io
73 stars 26 forks source link

Replace deprecated jQuery methods #545

Closed piotrbak closed 3 years ago

piotrbak commented 3 years ago

Before submitting an issue please check that you’ve completed the following steps:

Describe the bug When checking on WordPress 5.7 + SCRIPT_DEBUG defined, we're getting the following warnings: JQMIGRATE: jQuery.fn.removeAttr no longer sets boolean properties: disabled in pricing-modal.js JQMIGRATE: jQuery.fn.focus() event shorthand is deprecated in admin.js

To Reproduce Steps to reproduce the behavior:

  1. Update to WordPress 5.7 beta
  2. Add define( 'SCRIPT_DEBUG', true ); to wp-config.php
  3. Go to the Imagify settings with JS console opened and click on the What Plan do I need
  4. See an error

Expected behavior We should be compatible with the new jQuery version

Additional context Wasn't able to find any other deprecations but we should recheck this carefully.

Backlog Grooming (for WP Media dev team use only)

iCaspar commented 3 years ago

Reproduced ✅ The jQuery.fn.removeAttr appears on page load. In addition, on page load I get the following warning:

Source map error: Error: request failed with status 404
Resource URL: https://rocket.local/wp-content/plugins/imagify/assets/js/es6-promise.auto.js?ver=1616422431
Source Map URL: es6-promise.auto.map

The jQuery.fn.focus() warning is triggered when clicking the "What Plan do I Need" button.

Root Cause

  1. In the case of jQuery.fn.removeAttr the assets/js/pricing-modal.js is doing_it_wrong on L:244:
    $('#imagify-modal-checkout-btn').removeAttr('disabled').removeClass('imagify-button-disabled');

    incorrectly uses removeAttr(); it should be prop('disabled, false)

NOTE: Not in OP, and not currently causing a warning, but just above this on L:242 is also doing_it_wrong:

$('#imagify-modal-checkout-btn').attr('disabled', 'disabled').addClass('imagify-button-disabled');

should replace attr(...) with prop('disabled', true).

  1. In the case of jQuery.fn.focus() the assets/js/admin.js is using the deprecated focus() method on L:21. Updating to
    } ).trigger('focus').removeAttr( 'tabindex' ).addClass( 'modal-is-open' );

    resolves the deprecation.

There is a second use of focus() on L:21 -

} ).focus().removeAttr( 'tabindex' ).addClass( 'modal-is-open' );

Which should also be updated to:

} ).trigger('focus').removeAttr( 'tabindex' ).addClass( 'modal-is-open' );
  1. In the case of es6-promise.auto.map, the assets/js/es6-promise.auto.map.js includes a reference to this map file from what appears to be the https://github.com/components/es6-promise package, but the map file is not included in the imagify assets. Since the reference to the map file is for debugging in the other package, the reference to it can be removed from the imagify version of the file to clear the 404.

Following these changes, the grunt js task should be run to recompile the minified js files for production.

Estimate Effort: [S]