DevinVinson / WordPress-Plugin-Boilerplate

[WordPress] A foundation for WordPress Plugin Development that aims to provide a clear and consistent guide for building your plugins.
http://wppb.io
7.67k stars 2.25k forks source link

Separate settings functions in a separate class #84

Closed grappler closed 11 years ago

grappler commented 11 years ago

I was talking to @GaryJones at wceu and he mentioned placing the settings function in another class.

I thought it was good idea. So that is why I have done in on my fork. https://github.com/grappler/WordPress-Plugin-Boilerplate/tree/patch-2

What do you think?

GaryJones commented 11 years ago

Unsurprisingly, I'm a +1 for this general move. It keeps the main plugin class as a separate controlling entity of the whole plugin - not whatever the actual plugin does. It can hold the VERSION constant, a PLUGIN_SLUG constant (both which might be needed by other classes), and load in the text domain - tasks related to the plugin in general.

In a recent plugin I've just done, I actually took it further - a class that handles the options in terms of setting and getting from the database, and separate classes for the two admin pages that then utilised those values within admin page settings (the options handler class was then passed into the admin page classes via constructor dependency injection to keep everything clean). I'm not necessarily saying it should be applied here, but it's something to think about, since the abstraction can be seen even when there's only a single admin page. I should be able to create a new admin page and utilise existing settings values, without having to duplicate code or call across to the first admin page class.

In your case @grappler, the enqueue_admin_*() methods could also be moved across since they are targeted to only load on specific admin pages, and the $plugin_screen_hook_suffix dropped from the main class`.

tommcfarlin commented 11 years ago

This issue is timely - I've been receiving a few emails about how to begin incorporating settings pages and related functions into the plugin boilerplate.

Historically, I've been against this, because I felt like not all plugins need settings pages, so introducing one is introducing more for the developer to take a way than use; however, with the demand getting stronger for it, I'm happy to include it.

In a recent email, I did tell someone that I recommend including it in another class in the plugin for the sake of maintenance (and because I'm a fan of the SOLID principles, so - y'know - the whole single-responsibility idea).

Anyway, I like @GaryJones approach of separating classes because I think it's separating business logic from view logic, though for the first implementation, I may see about just getting @grappler's suggestion of getting a separate class into the plugin, then refactoring it from there.

This is all good stuff and I appreciate the approach and contribution.

@grappler: If you'll open a pull request, I'll be happy to merge it and then we can all begin the usual round of code reviews.

barryceelen commented 11 years ago

Along the same lines, @codebykat used kinda the same strategy in the Post By Email plugin, where the admin related stuff goes into a class-plugin-name-admin.php file, which can then be loaded if is_admin().

grappler commented 11 years ago

Ok, I have done a pull request. :smile:

GaryJones commented 11 years ago

I've been receiving a few emails about how to begin incorporating settings pages and related functions into the plugin boilerplate.

One thing I'd like to see if the admin page stuff is going to be pulled into another class, is a default implementation of the Settings API - registering settings, sections, fields etc. It's far easier to have the class setup following a good practice like this, and be able to just delete the class if it's not needed, than just having half the job done in registering an admin page with no content.

I'd also like to see a default implementation of contextual help added to the admin class. If it's as easy as editing content for basic help, then it's going to be added to more plugins than someone having to do it by themselves.

How about a look at suggesting class file names as well? class-plugin-slug-admin.php and class-plugin-slug-public.php for back-end and front-end would follow our existing terminology, and if there are more than one admin or public class, then suffixes can easily be added as needed to differentiate.

GaryJones commented 11 years ago

I am not sure this is the best solution for plugins. There is a Team working on improving it and try other solutions. I have have never used it a lot, personally I find simple links like I have implemented here better.

How the contextual help is presented (dropdown tab, popup-hover, modal etc.) is up to the core folks, but they're almost certainly going to have to keep the way of coding it the same, so what we can add in now would still be valid. They are also still at the UI stage, so there's a long way to go before they even think about getting it in to WP 3.8 - we shouldn't be waiting on them for that.

As to the benefit - that's the system that exists, it's in core, it's used by core, can be used by plugins and themes, and while I don't think it's used as much as it could be (that's one of the reasons for coming up with a different UI, and we probably aren't the target audience for that feature anyway), it is part of the current UI and we should be honouring that. Links like you've got within admin pages suffer from several drawbacks:

  1. Tooltips are only suitable for short snippets of help since it can be difficult to read, not contain markup and may disappear before someone has finished processing it.
  2. Accessing those tooltips (title attributes) may well be trickier for assistive technologies than just reading un-attributed plain text on screen. Introducing the help via data- attributes then requires the use of JavaScript to visually display it, giving another opportunity to deviate from the WP standard appearance.
  3. You've got links on screen that are most likely not links at all (# or javascript: or empty hrefs).

Remember, we're not catering for what works best for us, but what works best for the lowest common denominator of our users.