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

Add settings screen to toggle modules #30

Merged
merged 10 commits into from
Dec 6, 2021
Merged

Conversation

felixarntz
Copy link
Member

@felixarntz felixarntz commented Dec 2, 2021

This PR implements the performance plugin's settings screen to toggle the available modules on/off, as outlined in #27.

  • Implement a settings screen via a new /admin/load.php file where it displays all modules in the plugin's /modules directory, with a checkbox to toggle them on/off.
  • As part of the above, implement functions perflab_get_modules() and perflab_get_module_data() to read modules from a directory and parse their data, similar to how plugins are searched and parsed in WordPress core.
  • Implement a function perflab_get_focus_areas() for the available focus areas. These are hard-coded and can easily be expanded as more areas need to be defined.
  • Include test coverage for all new functions added, based on a test data modules directory with demo modules to be able to test the module functions independently of which real modules exist.
  • Load admin integration only if is_admin() to avoid the overhead and also clarify that those functions are not intended for frontend usage.

Screenshot with demo modules:
Screen Shot 2021-12-02 at 11 25 44 AM

Note that by default right now the screen will be empty, since there are no modules yet. For testing, you can easily recreate the experience from the above screenshot by changing https://github.com/WordPress/performance/pull/30/files#diff-cb909f9aba14cf3fa470fc35d1a245d296c4341882b13e95f46f16a4a82f2fdaR60 to $modules = perflab_get_modules( dirname( __DIR__ ) . '/tests/testdata/modules' );.

Fixes #27

@felixarntz felixarntz marked this pull request as ready for review December 2, 2021 23:08
@adamsilverstein
Copy link
Member

I tested this with #32 and it worked well in my testing, giving me the module as a setting:

image

* 'name'.
*/
function perflab_get_focus_areas() {
return array(
Copy link
Member

@ThierryA ThierryA Dec 6, 2021

Choose a reason for hiding this comment

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

I wonder if we shouldn't dynamically get the focuses from the modules header. On one hand it is more dynamic on the other it can easily result too undesired results due to typo or else. Thoughts?

Also, I do think that we may need an additional focus area for the "Infrastructure/API" modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think having these defined in a central location helps providing guidance on what to specify in the module header, and it also allows us to provide any extra information about a focus area (e.g. if in the future we wanted to add descriptions for each focus to the settings screen).

Adding a new focus area would be straightforward, it would only require a PR to expand this array (and it already requires a workflow anyway, as it will need a new GitHub label to be created as well).

Regarding something for "Infrastructure/API", I think that goes into the area of dependencies, or modules that go beyond a single focus. Maybe better to open a separate issue to discuss that, since it's a larger topic? Also it might be hard to evaluate this early as we aren't yet in a situation where it would be needed.

@felixarntz felixarntz changed the title Implement settings screen to toggle modules Add settings screen to toggle modules Dec 6, 2021
@felixarntz felixarntz added [Type] Feature A new feature within an existing module Infrastructure Issues for the overall performance plugin infrastructure labels Dec 6, 2021
@felixarntz felixarntz added this to the Initial plugin release milestone Dec 6, 2021
@felixarntz felixarntz merged commit 2176a10 into trunk Dec 6, 2021
@tillkruss tillkruss deleted the add/27-settings-screen branch March 7, 2022 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add settings page
4 participants