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

Use AJAX for activating features / plugins in Performance Lab #1646

Merged
merged 25 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
47f3938
Update print plugin progress indicator script function to support aja…
b1ink0 Nov 7, 2024
9d97f59
Add admin notice dynamically, Update to use async/await
b1ink0 Nov 8, 2024
6f172ad
Add settings URL of plugin in admin notice
b1ink0 Nov 11, 2024
337da6c
Move inline JavaScript to separate file
b1ink0 Nov 11, 2024
58c37d9
Refactor JavaScript code to use jQuery
b1ink0 Nov 11, 2024
a59909d
Update to use IIFE, Remove unnecessary doc comment
b1ink0 Nov 11, 2024
f9faaea
Merge branch 'trunk' into update/use-ajax-for-activate-plugin
b1ink0 Nov 11, 2024
770d462
Merge branch 'WordPress:trunk' into update/use-ajax-for-activate-plugin
b1ink0 Nov 12, 2024
e6710a3
Remove admin notice part, Remove constant, Use plugin_url for JS file…
b1ink0 Nov 12, 2024
c3d3588
Refactor to use vanilla JavaScript
b1ink0 Nov 12, 2024
fdf7e0b
Fix duplicate request on multiple click, Fix settings URL not showing…
b1ink0 Nov 12, 2024
b3ecc98
Remove wp-a11y enqueue, Revert changes to Speculative Loading plugin …
b1ink0 Nov 13, 2024
c713e47
Add REST API endpoints for activating plugin and getting plugin setti…
b1ink0 Nov 13, 2024
1ebe323
Merge branch 'trunk' into update/use-ajax-for-activate-plugin
b1ink0 Nov 13, 2024
c134fe2
Update REST API routes, Add validation to args of REST API request, R…
b1ink0 Nov 14, 2024
369487f
Update REST API routes, Update JavaScript code to use updated REST AP…
b1ink0 Nov 15, 2024
d84e9bd
Merge branch 'trunk' into update/use-ajax-for-activate-plugin
b1ink0 Nov 15, 2024
9bee27e
Fix inline comments and error messages
b1ink0 Nov 15, 2024
ce217ba
Add missing since, Remove unnecessary check in JavaScript
b1ink0 Nov 15, 2024
fa5d869
Add typing for featureInfo
westonruter Nov 15, 2024
3a333ae
Clarify description of slug arg
westonruter Nov 15, 2024
6250658
Use `plugin_dir_url()` for now.
felixarntz Nov 15, 2024
a8d89a8
Fix capability check on installation/activation endpoint.
felixarntz Nov 15, 2024
c593ae6
Use more appropriate HTTP response codes for different errors.
felixarntz Nov 15, 2024
e0691bf
Add missing require_once to prevent fatal error when installing plugi…
felixarntz Nov 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 59 additions & 39 deletions plugins/performance-lab/includes/admin/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@ function perflab_load_features_page(): void {

// Handle style for settings page.
add_action( 'admin_head', 'perflab_print_features_page_style' );

// Handle script for settings page.
add_action( 'admin_footer', 'perflab_print_plugin_progress_indicator_script' );
}

/**
Expand Down Expand Up @@ -230,6 +227,24 @@ function perflab_enqueue_features_page_scripts(): void {

// Enqueue the a11y script.
wp_enqueue_script( 'wp-a11y' );
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these enqueue now? as it already added through new script dependancy.

Copy link
Contributor Author

@b1ink0 b1ink0 Nov 12, 2024

Choose a reason for hiding this comment

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

We could remove it as it is not being used anywhere other than the AJAX script as it already added in its dependancy. Should I remove it then?


// Enqueue plugin activate AJAX script and localize script data.
wp_enqueue_script(
'perflab-plugin-activate-ajax',
PERFLAB_PLUGIN_DIR_URL . 'includes/admin/plugin-activate-ajax.js',
Copy link
Member

Choose a reason for hiding this comment

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

This PERFLAB_PLUGIN_DIR_URL is plugin_dir_url( PERFLAB_MAIN_FILE ), but I believe here it would be better to use plugins_url():

Suggested change
PERFLAB_PLUGIN_DIR_URL . 'includes/admin/plugin-activate-ajax.js',
plugins_url( 'includes/admin/plugin-activate-ajax.js', PERFLAB_MAIN_FILE ),

The reason is that this function allows for filtering as needed.

Copy link
Member

Choose a reason for hiding this comment

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

@westonruter I don't think we should randomly change it here though. If not using plugins_url() leads to a problem, let's discuss that in a separate issue and implement properly if needed.

Copy link
Member

Choose a reason for hiding this comment

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

I've filed #1647 for this.

array( 'jquery', 'wp-i18n', 'wp-a11y' ),
Copy link
Member

Choose a reason for hiding this comment

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

Related to @westonruter's previous feedback, it would be great not to use jQuery for the functionality, but rather vanilla JavaScript (e.g. fetch() function).

(string) filemtime( PERFLAB_PLUGIN_DIR_PATH . 'includes/admin/plugin-activate-ajax.js' ),
true
);

wp_localize_script(
'perflab-plugin-activate-ajax',
'perflabPluginActivateAjaxData',
array(
'ajaxUrl' => admin_url( 'admin-ajax.php' ),
'nonce' => wp_create_nonce( 'perflab_install_activate_plugin_ajax' ),
)
);
westonruter marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -290,6 +305,47 @@ function perflab_install_activate_plugin_callback(): void {
}
add_action( 'admin_action_perflab_install_activate_plugin', 'perflab_install_activate_plugin_callback' );

/**
* Callback for handling installation/activation of plugin using ajax.
*
* @since n.e.x.t
*/
function perflab_install_activate_plugin_ajax_callback(): void {
check_ajax_referer( 'perflab_install_activate_plugin_ajax', '_ajax_nonce' );

require_once ABSPATH . 'wp-admin/includes/plugin.php';
require_once ABSPATH . 'wp-admin/includes/plugin-install.php';
require_once ABSPATH . 'wp-admin/includes/class-wp-upgrader.php';
require_once ABSPATH . 'wp-admin/includes/class-wp-ajax-upgrader-skin.php';

if ( ! isset( $_POST['slug'] ) ) {
wp_send_json_error( __( 'Missing required parameter.', 'performance-lab' ), 400 );
}

$plugin_slug = perflab_sanitize_plugin_slug( wp_unslash( $_POST['slug'] ) );
if ( null === $plugin_slug ) {
wp_send_json_error( __( 'Invalid plugin.', 'performance-lab' ), 400 );
}

// Install and activate the plugin and its dependencies.
$result = perflab_install_and_activate_plugin( $plugin_slug );
if ( $result instanceof WP_Error ) {
wp_send_json_error( $result->get_error_message() );
}

$plugin_settings_url = perflab_get_plugin_settings_url( $plugin_slug );
if ( null !== $plugin_settings_url ) {
wp_send_json_success(
array(
'pluginSettingsURL' => esc_url( $plugin_settings_url ),
b1ink0 marked this conversation as resolved.
Show resolved Hide resolved
)
);
}

wp_send_json_success();
}
add_action( 'wp_ajax_perflab_install_activate_plugin', 'perflab_install_activate_plugin_ajax_callback' );
westonruter marked this conversation as resolved.
Show resolved Hide resolved

/**
* Callback function to handle admin inline style.
*
Expand Down Expand Up @@ -396,42 +452,6 @@ static function ( $name ) {
}
}

/**
* Callback function that print plugin progress indicator script.
*
* @since 3.1.0
*/
function perflab_print_plugin_progress_indicator_script(): void {
$js_function = <<<JS
function addPluginProgressIndicator( message ) {
document.addEventListener( 'DOMContentLoaded', function () {
document.addEventListener( 'click', function ( event ) {
if (
event.target.classList.contains(
'perflab-install-active-plugin'
)
) {
const target = event.target;
target.classList.add( 'updating-message' );
target.textContent = message;

wp.a11y.speak( message );
}
} );
} );
}
JS;

wp_print_inline_script_tag(
sprintf(
'( %s )( %s );',
$js_function,
wp_json_encode( __( 'Activating...', 'default' ) )
),
array( 'type' => 'module' )
);
}

/**
* Gets the URL to the plugin settings screen if one exists.
*
Expand Down
175 changes: 175 additions & 0 deletions plugins/performance-lab/includes/admin/plugin-activate-ajax.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
/**
* Handles activation of Performance Features (Plugins) using AJAX.
*/

/* global perflabPluginActivateAjaxData */
( function ( $ ) {
// @ts-ignore
const { i18n, a11y } = wp;
westonruter marked this conversation as resolved.
Show resolved Hide resolved
const { __, _x } = i18n;

/**
* Adds a click event listener to the plugin activate buttons.
*
* @param {MouseEvent} event - The click event object that is triggered when the user clicks on the document.
*
* @return {Promise<void>} - The asynchronous function returns a promise.
*/
b1ink0 marked this conversation as resolved.
Show resolved Hide resolved
$( document ).on(
'click',
'.perflab-install-active-plugin',
Copy link
Member

Choose a reason for hiding this comment

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

What if someone clicks this button while another request is currently in flight? Should it short-circuit so as to avoid duplicating requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think as the request could have been half finished we should not short-circuit the request. I think it will be better to just disable it until the request is finished. But as the click event is on the anchor tag we will have to add pointer-events: none CSS to the anchor link using JavaScript to disable it.

Copy link
Member

Choose a reason for hiding this comment

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

The pointer-events style wouldn't address the issue of the button being activated by keyboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the click event is on the anchor tag and not button we will have to add some other work around for disabling on keyboard one is setting its tabindex attribute to -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this but if user is already focused then tabindex will not work. One other way is to add a attribute like aria-disabled and then check on click event if it has that attribute then return from the execution. Should I add tabindex and aria-disabled both?

Copy link
Member

Choose a reason for hiding this comment

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

You could add the disabled attribute, but that wouldn't be good because it would lose the tab focus for users navigating with the keyboard. Instead of this, WordPress has style rules for when you add the disabled class to a button so that it will look disabled. Adding aria-disabled here too would be good. But then for how to detect whether request is currently in progress, you could just short-circuit the click handler if the button has the updating-message class, or if it has the disabled class, or if it has the aria-disabled attribute.

async function ( event ) {
// Prevent the default link behavior.
event.preventDefault();

// Get the clicked element as a jQuery object.
const target = $( this );

westonruter marked this conversation as resolved.
Show resolved Hide resolved
target
.addClass( 'updating-message' )
.text( __( 'Activating…', 'performance-lab' ) );

a11y.speak( __( 'Activating…', 'performance-lab' ) );

// Retrieve the plugin slug from the data attribute.
const pluginSlug = $.trim( target.attr( 'data-plugin-slug' ) );
b1ink0 marked this conversation as resolved.
Show resolved Hide resolved

// Send an AJAX POST request to activate the plugin.
$.ajax( {
// @ts-ignore
url: perflabPluginActivateAjaxData.ajaxUrl,
method: 'POST',
westonruter marked this conversation as resolved.
Show resolved Hide resolved
dataType: 'json',
data: {
action: 'perflab_install_activate_plugin',
slug: pluginSlug,
// @ts-ignore
_ajax_nonce: perflabPluginActivateAjaxData.nonce,
},
success( responseData ) {
if ( ! responseData.success ) {
showAdminNotice(
__(
'There was an error activating the plugin. Please try again.',
'performance-lab'
)
);

target
.removeClass( 'updating-message' )
.text( __( 'Activate', 'performance-lab' ) );

return;
}

// Replace the 'Activate' button with a disabled 'Active' button.
target.replaceWith(
$( '<button>', {
type: 'button',
class: 'button button-disabled',
disabled: true,
text: __( 'Active', 'performance-lab' ),
} )
);

const pluginSettingsURL =
responseData?.data?.pluginSettingsURL;

// Select the container for action buttons related to the plugin.
const actionButtonList = $(
`.plugin-card-${ pluginSlug } .plugin-action-buttons`
);

if ( pluginSettingsURL && actionButtonList ) {
// Append a 'Settings' link to the action buttons.
actionButtonList.append(
$( '<li>' ).append(
$( '<a>', {
href: pluginSettingsURL,
text: __( 'Settings', 'performance-lab' ),
} )
)
);
}

showAdminNotice(
__( 'Feature activated.', 'performance-lab' ),
'success',
pluginSettingsURL
);
Copy link
Member

Choose a reason for hiding this comment

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

In the demo video you showed that this notice is causing layout shifts, resulting in you accidentally clicking the title of a plugin instead of the Activate button. I don't think an admin notice is even needed here, because the button transitions from Activate to Activating... to Active.

},
error() {
showAdminNotice(
__(
'There was an error activating the plugin. Please try again.',
'performance-lab'
)
);

target
.removeClass( 'updating-message' )
.text( __( 'Activate', 'performance-lab' ) );
},
} );
}
);

/**
* Displays an admin notice with the given message and type.
*
* @param {string} message - The message to display in the notice.
* @param {string} [type='error'] - The type of notice ('error', 'success', etc.).
* @param {string} [pluginSettingsURL] - Optional URL for the plugin settings.
*/
function showAdminNotice(
message,
type = 'error',
pluginSettingsURL = undefined
) {
a11y.speak( message );

// Create the notice container elements.
const notice = $( '<div>', {
class: `notice is-dismissible notice-${ type }`,
} );

const messageWrap = $( '<p>' ).text( message );

// If a plugin settings URL is provided, append a 'Review settings.' link.
if ( pluginSettingsURL ) {
messageWrap
.append( ` ${ __( 'Review', 'performance-lab' ) } ` )
.append(
$( '<a>', {
href: pluginSettingsURL,
text: __( 'settings', 'performance-lab' ),
} )
)
.append( _x( '.', 'Punctuation mark', 'performance-lab' ) );
}

const dismissButton = $( '<button>', {
type: 'button',
class: 'notice-dismiss',
click: () => notice.remove(),
} ).append(
$( '<span>', {
class: 'screen-reader-text',
text: __( 'Dismiss this notice.', 'performance-lab' ),
} )
);

notice.append( messageWrap, dismissButton );

const noticeContainer = $( '.wrap.plugin-install-php' );

if ( noticeContainer.length ) {
// If the container exists, insert the notice after the first child.
noticeContainer.children().eq( 0 ).after( notice );
} else {
$( 'body' ).prepend( notice );
}
}

westonruter marked this conversation as resolved.
Show resolved Hide resolved
// @ts-ignore
} )( window.jQuery );
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to make jQuery a dependency? Would plain JS not be feasible?

Copy link
Member

Choose a reason for hiding this comment

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

+1, it would be great to use vanilla JavaScript rather than relying on jQuery, to set a good example for performance.

3 changes: 2 additions & 1 deletion plugins/performance-lab/includes/admin/plugins.php
Original file line number Diff line number Diff line change
Expand Up @@ -426,8 +426,9 @@ function perflab_render_plugin_card( array $plugin_data ): void {
);

$action_links[] = sprintf(
'<a class="button perflab-install-active-plugin" href="%s">%s</a>',
'<a class="button perflab-install-active-plugin" href="%s" data-plugin-slug="%s">%s</a>',
esc_url( $url ),
esc_attr( $plugin_data['slug'] ),
esc_html__( 'Activate', 'default' )
);
} else {
Expand Down
1 change: 1 addition & 0 deletions plugins/performance-lab/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
define( 'PERFLAB_VERSION', '3.5.1' );
define( 'PERFLAB_MAIN_FILE', __FILE__ );
define( 'PERFLAB_PLUGIN_DIR_PATH', plugin_dir_path( PERFLAB_MAIN_FILE ) );
define( 'PERFLAB_PLUGIN_DIR_URL', plugin_dir_url( PERFLAB_MAIN_FILE ) );
Copy link
Member

Choose a reason for hiding this comment

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

If my suggestion above is accepted, this can then be removed.

Suggested change
define( 'PERFLAB_PLUGIN_DIR_URL', plugin_dir_url( PERFLAB_MAIN_FILE ) );

Copy link
Member

Choose a reason for hiding this comment

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

See above, let's discuss separately, it's not really relevant for this enhancement.

Copy link
Member

Choose a reason for hiding this comment

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

At least let's avoid introducing a new constant in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair, we can simply use plugin_dir_url( PERFLAB_MAIN_FILE ) in the wp_enqueue_script() call directly. I'm personally not a fan of having constants for all this either, particularly as the values are filterable, like you're saying.

Copy link
Member

@westonruter westonruter Nov 11, 2024

Choose a reason for hiding this comment

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

I've filed #1647 for the filtering.

define( 'PERFLAB_SCREEN', 'performance-lab' );

// If the constant isn't defined yet, it means the Performance Lab object cache file is not loaded.
Expand Down
Loading