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

[Develop] Turn parent menu slug into a config variable #221

Closed
wants to merge 6 commits into from

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Sep 7, 2014

Allows for setting of the parent menu item from the $config array

@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 2, 2015

@thomasgriffin Any feedback on this ?

@jrfnl jrfnl changed the title Turn parent menu slug into a config variable [Develop] Turn parent menu slug into a config variable Jan 13, 2015
@senlin
Copy link

senlin commented Jan 14, 2015

This looks great, would be awesome if it can be integrated. Thanks!

@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 14, 2015

@senlin Glad you like it ;-)

@thomasgriffin I kind of have the feeling that you may be hesitant about this PR because it builds on the changes from @zackkatz's PR #188 which were merged in develop, but not in master because of the theme check disallowing the use of add_submenu_page().

I think I can provide a solution.

A while ago, I send in PR #219 with some changes to allow for overloading of the class. If that PR would be merged, we can then leave the 'old' method in the base class which would be used by themes.
For plugins which may want to change where the menu is added, an overloaded class can be added which adjusts the admin_menu() method to use add_submenu_page() instead.

I might need to still make some small changes to the code in #219 to make this more intuitive for less experienced programmers, but would like your opinion before I spend a lot (more) time on something which may never get accepted.

So what do you think ?

…ion into develop-parent-slug-in-config

Conflicts:
	class-tgm-plugin-activation.php
@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 15, 2015

Also: the current getting of the admin page with the code in the develop branch what with the hard-coded themes.php, only works because of a recognized bug in WP. We can't count on the bug staying in.
Ref: https://core.trac.wordpress.org/ticket/28821

@GaryJones
Copy link
Member

+1 for abstracting 'themes.php'.

0 (but willing to be swayed) for making them properties as well. What's the reasoning there? To be able to subclass? (If so, how does that handle with the main class being a singleton?)

@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 23, 2015

0 (but willing to be swayed) for making them properties as well. What's the reasoning there? To be able to subclass? (If so, how does that handle with the main class being a singleton?)

@GaryJones I followed the logic which was already in place - the TGM_Plugin_Activation::config() method currently sets all config variables passed as properties, i.e. the properties have to be there.

@GaryJones
Copy link
Member

+1 for being consistent then :-)

@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 23, 2015

Related #65

@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 23, 2015

Related 44228af and 5c76406 which seem to be reversed/taken out by c44d6aa

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