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

Allowing to specify admin menu link position fix #65

Closed
wants to merge 2 commits into from

Conversation

icecaster
Copy link

This patch fixes admin linking issues by adding a 'parent_admin_url' key to the strings array.

this way the admin_url() function can function properly with urls like admin.php?page=mytheme_options.

… issues with admin.php?page=mytheme_otpions like parent slugs
@GaryJones
Copy link
Member

-1 on the code in this pull request.

You've added an unused parent_slug array key.
You've changed one of the wp_nonce keys from tgmpa-install to tgmpa.
What you've added isn't a string (in the context of it being displayed text), but a config value, and as such, it should be created as a class property, and added to the whitelist of assignable properties on L646.

You should be able to edit this pull request, rather than creating a new one each time.

@thomasgriffin
Copy link
Contributor

I am guessing this commit is to help in preparation for your previously closed commit? Just want to make sure because if not, I'm not sure I see what the problem is with the current implementation? :-)

@icecaster
Copy link
Author

@thomasgriffin

the current implementation has the plugins page hard coded under the appearance tab in the admin which will break the links when using a custom parent slug.

@GaryJones
Thanks for the feedback.

I'm a little confused by these github pull requests and merging your changes to the dev branch back into my patch.
Your suggestions make sense I will try to get them implemented and update here.

@thomasgriffin
Copy link
Contributor

Ok, that's what I suspected you were talking about.

Just to be sure, as Gary said, by default this should be under the Appearance tab. This makes sure that we are following along with the theme review standards, which states that pages like this should be under that tab. Obviously we can make it filterable for users, but make sure the default is under Appearance.

Also, make sure to update the references within the TGMPA_List_Table class, specifically references from the $_tgmpa object. :)

@icecaster
Copy link
Author

Ok, just getting my head wrapped around this git stuff ;)

Have a look at this commit. Would this work for you?

Thanks

@GaryJones
Copy link
Member

Thanks for persisting!

I can see one minor issue with the vertical alignment of a comment (use spaces inside a line - only use tabs at the start to amend the indentation), and @var descriptions should start with a capital letter (and a full stop, but we're not consistent on that yet, and will likely switch to @type at some point when the PSR becomes stable.)

The example file should also be updated to include the commented out new config variables, with the default values.

At first glance, the code logic seems to be sensible, so I'll let Thomas test and commit it.
Thanks again!

@thomasgriffin
Copy link
Contributor

I'm closing this request as there has not been action on it in a month. However, I do plan to implement this idea and will give credit in the next version of the class.

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