Directory

Fixed :: limit listening to AJAX actions to set `Prefers reduced moti… by bplv112 · Pull Request #5536 · WordPress/wordpress-develop · GitHub
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

Fixed :: limit listening to AJAX actions to set `Prefers reduced moti… #5536

Closed

Conversation

bplv112
Copy link

@bplv112 bplv112 commented Oct 20, 2023

  1. The newly added jQuery function on 6.4 RC 1 listens to all completed Ajax requests. ef54f4e#diff-b9e9194f24d2a3548ae0b16b6bde5e0c7756a2b7668bed4d1040093e00550bf6R2201

  2. The function expects string but if the data is an object it throws an error for other requests as well. ef54f4e#diff-b9e9194f24d2a3548ae0b16b6bde5e0c7756a2b7668bed4d1040093e00550bf6R2203

Trac ticket: https://core.trac.wordpress.org/ticket/59689


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@@ -2220,8 +2220,14 @@ $( function( $ ) {
// Listen for jQuery AJAX events.
( function( $ ) {
$( document ).ajaxComplete( function( event, xhr, settings ) {

// Return early if this is not the plugin-install.php page.
if ( ! $( document.body ).hasClass( 'plugin-install-php' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there are more standard ways in WordPress to check what a page is, see the followiing JS 'global' variables:

window.adminpage will return 'plugin-install-php' which is the current page admin body CSS class
window.pagenow will return 'plugin-install' which is che current screen ID

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afercia Thank you for the suggestion. I've updated it to window.pagenow

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bplv112 you're welcome and thanks for the PR!

// Check if this is the 'search-install-plugins' request.
if ( settings.data && settings.data.includes( 'action=search-install-plugins' ) ) {
if ( settings.data && typeof settings.data === "string" && settings.data.includes( 'action=search-install-plugins' ) ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( settings.data && typeof settings.data === "string" && settings.data.includes( 'action=search-install-plugins' ) ) {
if ( settings.data && typeof settings.data === 'string' && settings.data.includes( 'action=search-install-plugins' ) ) {

Just to keep consistency, I think you can use 'string' instead of "string" because the single quote is a more strict sting and everywhere uses a single quote of this file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rudlinkon Thank you for reviewing the PR.
I've updated it as per your suggestion

if ( window.pagenow !== 'plugin-install' ) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this change fixes the problem. The ajaxComplete is still listened to; it just exits under different conditions. Wouldn't it make more sense to move the page check as a wrapper before the listener?

if ( window.pagenow === 'plugin-install' ) {
    $( document ).ajaxComplete( function( event, xhr, settings ) {
        // etc.
    });
}

@joedolson
Copy link
Contributor

@joedolson joedolson closed this Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants