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

Replace deprecated jQuery methods #545

Closed
4 tasks done
piotrbak opened this issue Feb 23, 2021 · 1 comment · Fixed by #575
Closed
4 tasks done

Replace deprecated jQuery methods #545

piotrbak opened this issue Feb 23, 2021 · 1 comment · Fixed by #575
Assignees
Labels
Milestone

Comments

@piotrbak
Copy link

piotrbak commented Feb 23, 2021

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

  • Made sure you’re on the latest version
  • Used the search feature to ensure that the bug hasn’t been reported before

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)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@iCaspar
Copy link
Contributor

iCaspar commented Mar 22, 2021

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]

@iCaspar iCaspar added effort [S] 1-2 days of estimated development time and removed needs: grooming labels Mar 22, 2021
@remyperona remyperona added this to the 1.9.16 milestone Apr 12, 2021
@remyperona remyperona modified the milestones: 1.9.16, 1.9.15 May 4, 2021
@mostafa-hisham mostafa-hisham self-assigned this Jul 6, 2021
@remyperona remyperona linked a pull request Jul 7, 2021 that will close this issue
mostafa-hisham added a commit that referenced this issue Jul 13, 2021
* replace deprecated jquery functions
* run grunt js
* fix more deprecated jquery

Co-authored-by: Caspar Green <[email protected]>
@iCaspar iCaspar mentioned this issue Jul 15, 2021
iCaspar added a commit that referenced this issue Jul 15, 2021
* replace deprecated jquery functions
* run grunt js
* fix more deprecated jquery

Co-authored-by: Caspar Green <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants