Skip to content

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
@github-actions
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 <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: ju1ius <[email protected]>

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.

@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 [Plugin] Performance Lab 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.

@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.

@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 requested a review from joemcgill May 16, 2024 18:48
@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
@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

[Plugin] Performance Lab 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