Abstract notion of AMP Actions #725
Conversation
|
|
||
| require_once ( AMP__DIR__ . '/includes/actions/class-amp-actions.php' ); | ||
|
|
||
| class AMP_Frontend_Actions extends AMP_Actions { |
There was a problem hiding this comment.
I'm not sure if this class needs to extend AMP_Actions since it won't use any of the abstract class' methods.
There was a problem hiding this comment.
Agree. Made it standalone.
|
|
||
| class AMP_Frontend_Actions extends AMP_Actions { | ||
|
|
||
| public static function register_frontend_actions_filter() { |
There was a problem hiding this comment.
For consistency, we should name this register as well (or register_hooks if we want to be more descriptive)
There was a problem hiding this comment.
Agree; register_hooks() is more descriptive.
| add_action( 'wp_head', 'AMP_Frontend_Actions::amp_frontend_add_canonical' ); | ||
| } | ||
|
|
||
| public static function amp_frontend_add_canonical() { |
There was a problem hiding this comment.
We can remove the amp_frontend_ prefix
amedina
changed the title
There was a problem hiding this comment.
Spotted two very minor spacing issues.
Also minor, but the only other thing I wanted to flag is that we're not actually using the inheritance/base classes right now so maybe better to leave them out in this PR and introduce them when we actually start to use it. If you have other PRs ready or pending that rely on this, then okay to leave.
amp.php
Outdated
| @@ -21,6 +21,9 @@ | |||
| require_once( AMP__DIR__ . '/includes/settings/class-amp-customizer-settings.php' ); | |||
| require_once( AMP__DIR__ . '/includes/settings/class-amp-customizer-design-settings.php' ); | |||
|
|
|||
| require_once ( AMP__DIR__ . '/includes/actions/class-amp-frontend-actions.php' ); | |||
| <?php | ||
| // Callbacks for adding AMP-related things to the main theme | ||
|
|
||
| require_once ( AMP__DIR__ . '/includes/actions/class-amp-actions.php' ); |
…used currently and will be added in subsequent PR
amedina
added
Enhancement
| } | ||
|
|
||
| public static function add_canonical() { | ||
| if ( false === apply_filters( 'add_canonical_link', true ) ) { |
There was a problem hiding this comment.
This change here renamed the filter from amp_frontend_show_canonical to add_canonical_link. This needs to be reverted. I think this needs to be reverted.
Revert #725: Abstract notion of AMP Actions
This PR adds an AMP Actions class hierarchy to differentiate between the actions applied in paired mode vs. canonical mode (coming up), and also between different content types such as posts, pages (coming up), etc.