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

Create settings link for the plugin in the plugin listing page #298

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

namithj
Copy link
Contributor

@namithj namithj commented Feb 19, 2025

Pull Request

What changed?

Create settings link for the plugin in the plugin listing page

Why did it change?

Add settings link for the plugin in the plugin listing page for ease of use

Did you fix any specific issues?

#295

CERTIFICATION

By opening this pull request, I do agree to abide by the Code of Conduct and be bound by the terms of the Contribution Guidelines in effect on the date and time of my contribution as proven by the revision information in GitHub.

Create settings link for the plugin in the plugin listing page
@namithj namithj requested a review from a team February 19, 2025 19:44
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Signed-off-by: Namith Jawahar <48271037+namithj@users.noreply.github.com>
Copy link
Contributor

@afragen afragen left a comment

Choose a reason for hiding this comment

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

There's no need to remove then re-add hr deactivate link. Simply use an array_merge() and put the settings first.

Settings usually comes first or last in the links array.

@namithj
Copy link
Contributor Author

namithj commented Feb 20, 2025

Putting settings link first would simplify things but I checked a few plugins and they seem to retain the deactivate link in the first position.

If you have a plugin like PCP it adds the "check plugin" link to the end, making the settings link go to the last position when we add it.

Forcing it just after the deactivate seemed like a better choice. I do see a bug though with multisite when there might not be a deactivate link at all for some users. So we need to update the logic to check if there is a deactivate link and then swap OR just put the settings link at the beginning without complicating things.

@costdev
Copy link
Contributor

costdev commented Feb 20, 2025

Unless I'm mistaken, I think @afragen is suggesting this instead:

$settings_url = network_admin_url( 'index.php?page=aspireupdate-settings' );
return array_merge(
  [
    'settings' => '<a href="' . esc_url( $settings_url ) . '">' . __( 'Settings', 'aspireupdate' ) . '</a>',
  ],
  $links
);

And leaving deactivate wherever it is, if it's in the $links array at all (which would avoid the multisite issue too)

@costdev
Copy link
Contributor

costdev commented Feb 20, 2025

To my knowledge, there's no established best practice for which order a settings link appears. It's mostly more important that it's in there at all.

@namithj
Copy link
Contributor Author

namithj commented Feb 20, 2025

Yes, we can do that. No need to complicate it further by adding a check for deactivate link.

Update the Setting link position to just insert it at the beginning of the links
@namithj namithj requested a review from afragen February 20, 2025 18:08
@namithj namithj merged commit 3541d49 into aspirepress:main Feb 20, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants