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

Scope module assets for custom status #565

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cojennin
Copy link
Contributor

@cojennin cojennin commented Dec 9, 2019

This is based on work from @justnorris in #487.

It continues the work of scoping module assets. It reintroduces the EF_Module_With_View class from #487, but holds off on deprecating is_whitelisted_functional_view and is_whitelisted_settings_view (which will be deprecated once all modules have been migrated).

It also introduces a small change to Block_Editor_Compatible, enabling the registering/de-registering of multiple actions for the same hook

Also, holding off on altering any usages of disable_custom_statuses_for_post_type for this PR, as it caused issues with the original PR

@WPprodigy
Copy link
Contributor

Multiple inheritance patterns like this could get pretty messy down the road.

If a set of related helper methods are needed for specific modules, what do you think about using a trait instead? https://www.php.net/manual/en/language.oop5.traits.php

@cojennin
Copy link
Contributor Author

cojennin commented Jan 16, 2020

@WPprodigy Sure, I don't have a strong stance on the OOP mechanisms. Traits seem less arduous so I'm good with that

@cojennin cojennin force-pushed the scope-module-assets-custom-status branch from d1d018b to 77d4fc2 Compare January 16, 2020 15:05
@WPprodigy
Copy link
Contributor

WPprodigy commented Jan 16, 2020

Looking better. I have a couple of thoughts:

  1. I'm not a big fan of the current GB compatibility implementation. Seems to obfuscate quite a bit with little advantage and very limited flexibility (as seen here with you needing to extend how it works). I think I'm going to look into removing some of the abstraction layers, and give Modules more power over how it supports the block editor. This should help ease the efforts with these cleanups as well.

  2. The decision process for where a module decides if a script should be enqueued or not is kind of all over the place. It would be nice if we used the init() method as a very high-level overview of the module. Instead of tons of hooks inside of it. In other words, more descriptive wrapping methods like this:

function init() {
  $this->register_custom_statuses_taxonomy();
  $this->configure_settings_page();
  $this->integrate_custom_statuses();
}

Then configure_settings_page() would contain secondary level conditionals to control output:

function configure_settings_page() {
  add_action( 'admin_init', array( $this, 'register_settings' ) );

  if ( $this->is_current_module_settings_view() ) {
    // Or just the direct hook here if it's small / simple logic.
    $this->enqueue_settings_scripts();
  }
}

After level 1 (init()), wrapper methods could be used sparingly / only when it makes sense.

The benefit to this type of cleanup will be that we can clearly read from the top down (outline->chapter->paragraph), and decisions on whether or not something needs to be loaded will be more obvious.

@cojennin cojennin force-pushed the scope-module-assets-custom-status branch 4 times, most recently from 514bc96 to 601624c Compare February 13, 2020 20:25
@cojennin cojennin force-pushed the scope-module-assets-custom-status branch from 601624c to 0348c3e Compare February 13, 2020 21:06
@cojennin
Copy link
Contributor Author

The decision process for where a module decides if a script should be enqueued or not is kind of all over the place. It would be nice if we used the init() method as a very high-level overview of the module. Instead of tons of hooks inside of it.

@WPprodigy agreed, could use some TLC.

I think a series of PR's would make things easier. Can move and rearrange a little bit at the time, first by focusing on separating module settings page stuff and non-settings page module stuff.

So you'd end up with something like

function init() {
  $this->configure_module();
  $this->configure_module_settings_page();
}

and go from there (separating it out more as needed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants