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

Updated code to display plugin settings link in the features screen and fix responsive layout for mobile #1208

Conversation

ashwinparthasarathi
Copy link
Contributor

@ashwinparthasarathi ashwinparthasarathi commented May 7, 2024

Summary

Fixes #1137

Relevant technical choices

  1. Made changes in the Performance Features admin page code.
  2. Updated the plugin card rendering to display the plugin settings link if the plugin is active and if it has a settings page.

Result

  1. Currently shows the Settings link for the 2 modules that have it, Speculative Loading & Modern Image Formats
    image

Update: See #1208 (comment) for how the responsive layout is also fixed for mobile.

@ashwinparthasarathi ashwinparthasarathi marked this pull request as ready for review May 8, 2024 10:42
Copy link

github-actions bot commented May 8, 2024

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @benoitfouc.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: benoitfouc.

Co-authored-by: ashwinparthasarathi <ashwinparthasarathi@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: ju1ius <ju1ius@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Could the Settings link be moved after the Learn More link? Since not all plugins have Settings links, this will allow the Learn More link to be consistently after the button.

includes/admin/plugins.php Outdated Show resolved Hide resolved
includes/admin/plugins.php Outdated Show resolved Hide resolved
includes/admin/plugins.php Outdated Show resolved Hide resolved
includes/admin/plugins.php Outdated Show resolved Hide resolved
includes/admin/plugins.php Outdated Show resolved Hide resolved
includes/admin/plugins.php Outdated Show resolved Hide resolved
@westonruter
Copy link
Member

Something else to ponder: upon activating a plugin which has Settings, it would be a nice touch to do some yellow-fade or some other styling effect to highlight the fact that there is a Settings link now after the page reloaded which wasn't there before. Not sure what would be the most in-line with existing core designs.

@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature Performance Lab Plugin Issue relates to work in the Performance Lab Plugin only labels May 8, 2024
@westonruter westonruter added this to the performance-lab 3.1.0 milestone May 8, 2024
@ashwinparthasarathi
Copy link
Contributor Author

Could the Settings link be moved after the Learn More link? Since not all plugins have Settings links, this will allow the Learn More link to be consistently after the button.

Understood, made changes to move the link down as the last in the action items.

Something else to ponder: upon activating a plugin which has Settings, it would be a nice touch to do some yellow-fade or some other styling effect to highlight the fact that there is a Settings link now after the page reloaded which wasn't there before. Not sure what would be the most in-line with existing core designs.

If you could point me to a reference, I can figure out an appropriate design cue to highlight the settings link freshly added.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

@ashwinparthasarathi @westonruter, I believe we should postpone this to the next release. In issue #1032, we're investigating the activation of all active plugins (whether stable or experimental). Consequently, we would need to refactor this code significantly. Do these code changes support the activation of multiple plugins?

@westonruter
Copy link
Member

In issue #1032, we're investigating the activation of all active plugins (whether stable or experimental). Consequently, we would need to refactor this code significantly. Do these code changes support the activation of multiple plugins?

@mukeshpanchal27 No, it doesn't support multiple plugin activation, since this screen doesn't support that yet. But I don't think it would be so relevant as then all of the settings links would appear together all at once. I think we can land this in 3.1.0 and then refactor as part of the bulk-activation and also Ajax activation.

includes/admin/load.php Outdated Show resolved Hide resolved
includes/admin/load.php Outdated Show resolved Hide resolved
includes/admin/load.php Outdated Show resolved Hide resolved
includes/admin/plugins.php Outdated Show resolved Hide resolved
includes/admin/plugins.php Outdated Show resolved Hide resolved
includes/admin/load.php Outdated Show resolved Hide resolved
@westonruter
Copy link
Member

Just noticing this styling issue, where the Settings link is overflowing the card container:

image

@westonruter
Copy link
Member

westonruter commented May 16, 2024

Fixed styling issue in c7ba02c:

Screen.recording.2024-05-16.10.59.14.webm

I also noticed a separate issue where the plugin cards fail to render properly on mobile, but I'll address that in a follow-up PR. Actually, I went ahead and did it here: 57e0f8e

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Great! This will really improve discoverability of the features' settings.

@westonruter
Copy link
Member

With 57e0f8e, compare the responsive layout of the feature cards:

Before

Screen.recording.2024-05-16.11.45.09.webm

After

Screen.recording.2024-05-16.11.45.57.webm

Take note of the addition of the Settings link here, including how a divider appears when the links are presented horizontally.

@westonruter westonruter changed the title Updated code to display plugin settings link in the features screen Updated code to display plugin settings link in the features screen and fix responsive layout for mobile May 16, 2024
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice job.

@westonruter westonruter merged commit 074f962 into WordPress:trunk May 16, 2024
18 checks passed
@ashwinparthasarathi ashwinparthasarathi deleted the add/plugin-settings-link-in-features-screen branch May 17, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Lab Plugin Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add settings links to Performance features screen
5 participants