Omit admin pointer for new plugin if plugin is already active #2143
Conversation
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
ShyamGadde
added
the
[Plugin] Performance Lab
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #2143 +/- ##
==========================================
+ Coverage 68.74% 68.91% +0.16%
==========================================
Files 91 91
Lines 7996 8033 +37
==========================================
+ Hits 5497 5536 +39
+ Misses 2499 2497 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ); | ||
|
|
||
| // Only show if the feature plugin is not already active and not installed. | ||
| if ( ! in_array( 'view-transitions', $installed_plugin_slugs, true ) ) { |
There was a problem hiding this comment.
Similar to SPECULATION_RULES_VERSION, we should also check for VIEW_TRANSITIONS_VERSION here.
The same applies to the No-cache BFCache plugin, but I noticed it doesn’t currently define a version: https://github.com/westonruter/nocache-bfcache/blob/main/nocache-bfcache.php.
@westonruter Do you think we should define a version there as well, similar to how other Performance Lab plugins handle it?
There was a problem hiding this comment.
It does define a version, actually: https://github.com/westonruter/nocache-bfcache/blob/1f7118afee2dbdc43fc1642a7eac4a8ab81911c5/nocache-bfcache.php#L37
This constant is being used currently by the PL plugin when constructing the meta generator tag.
performance/plugins/performance-lab/load.php
Line 117 in 3d88f7b
There was a problem hiding this comment.
Similar to
SPECULATION_RULES_VERSION, we should also check forVIEW_TRANSITIONS_VERSIONhere.
I initially considered using constants (as seen in commit 4a0fb05) to check if the plugin is active. However, this approach only accounts for active plugins and does not handle the case where the plugin is installed but deactivated.
To address both scenarios (active plugins and installed-but-deactivated plugins), I opted to use the plugin slug in the subsequent commit. This ensures the pointer is not shown in either case.
If you still feel that the constant-based approach is preferable, I’m happy to revert to the earlier implementation. Let me know your thoughts!
cc @westonruter
There was a problem hiding this comment.
@ShyamGadde I think what you've done makes sense. The admin pointer should be be shown if a feature plugin has been installed or activated, since either is a signal that the use is already aware of the feature and doesn't need the pointer to inform them of anything.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@ShyamGadde there's one more scenario that I think would make sense to include here: what if someone has Performance Lab installed and they already have View Transitions installed. What if they then uninstall View Transitions? Would this not then cause the View Transitions admin pointer to immediately get displayed? If so, then I think we should proactively add the View Transitions admin pointer to the list of dismissed pointers, similar to how we dismiss all pointers when landing on the Performance screen. |
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
|
@ShyamGadde I did some refactoring to improve coverage and make it so that there was only one call to |
| $installed_plugins = get_plugins(); | ||
| if ( | ||
| defined( 'SPECULATION_RULES_VERSION' ) | ||
| isset( $installed_plugins['speculation-rules/load.php']['Version'] ) |
There was a problem hiding this comment.
This change allows for the code to be tested without having to define the constant by loading the plugin.
| if ( ! in_array( $hook_suffix, array( 'index.php', 'plugins.php' ), true ) ) { | ||
|
|
||
| // And if we're on the Performance screen, automatically dismiss the pointers. | ||
| if ( isset( $_GET['page'] ) && PERFLAB_SCREEN === $_GET['page'] ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended |
There was a problem hiding this comment.
This turns out to not have been ideal. It should have used $hooks_suffix, to see if it is equal to 'settings_page_' . PERFLAB_SCREEN.
| * Requires at least: 6.6 | ||
| * Requires PHP: 7.2 | ||
| * Version: 1.5.0 | ||
| * Version: 1.6.0 |
There was a problem hiding this comment.
This version bump was required in order to test the new admin pointer.
|
Build for testing: performance-lab.zip |
| // Pointer Options. | ||
| const options = { | ||
| content: <?php echo wp_json_encode( '<h3>' . esc_html( $args['heading'] ) . '</h3>' . wp_kses( $args['content'], $wp_kses_options ) ); ?>, | ||
| content: <?php echo wp_json_encode( '<h3>' . esc_html( $args['heading'] ) . '</h3>' . wp_kses( $args['content'], $wp_kses_options ), JSON_HEX_TAG | JSON_UNESCAPED_SLASHES ); ?>, |
There was a problem hiding this comment.
|
Some last sanity checks… When installing the plugin for the first time, I get this the initial pointer as expected: 
If I have already had the plugin installed and the initial pointer was dismissed, then upgrading to the new version shows the expected pointers: 
Upon updating Speculative Loading to the latest build, I then its specific pointer: 
|
3 participants
Summary
Fixes #2136