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

#849 Add paired mode custom templates #856

Merged
merged 19 commits into from Jan 15, 2018

Conversation

Projects
None yet
3 participants
@kaitnyl
Copy link
Contributor

commented Jan 11, 2018

This PR will allow users to specify a custom AMP template folder from within their own theme for #849.

===

Completed

  • Using "paired mode," themes can specify an alternative path to use for AMP templates (example below).
  • Templates in the custom directory will replace the entire "paired mode" template system in our plugin. The template hierarchy loaded is hooked into WP's template_include in order to follow typical template schemas.
  • Callback added to further control custom template usage when referencing custom directory (example below).
  • All "default" template output still available when not using a custom template folder.
  • Toggle within Customizer still available when using custom templates.

Testing example inside a theme:

add_theme_support( 'amp', array(
	'template_dir'       => 'amp-templates',
	'available_callback' => function() {
		return is_single(); // Change this up to enable/disable custom templates per $post
	},
) );

Updated as of b10c0fe.

===

In question

  • Update amp_prepare_render() to prevent calling add_action( 'template_redirect', 'amp_render' )

This part of the acceptance criteria may not be needed (but could be useful for clarity)? Before we get to amp_prepare_render(), we're initially inside amp_maybe_add_actions() and that will prevent calling amp_prepare_render() depending on this line.

ThierryA and others added some commits Dec 20, 2017

Merge pull request #853 from eshannon3/eshannon3/fix-header-id to 0.6…
… branch

Remove erroneous hash from id on amp-wp-header
* @since 0.7
* @var bool
*/
public $active;

This comment has been minimized.

Copy link
@kaitnyl

kaitnyl Jan 11, 2018

Author Contributor

I feel the term "active" might be too ambiguous here. This is what the callback property within the theme support addition controls, and is used when we do have a custom template included but want to control if it should be AMP or default WP templates (since the custom will try to follow WP template hierarchy and we may not always want it to).

@kaitnyl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

@ThierryA and @westonruter, would either of you mind reviewing this PR for #849? 😃

@kaitnyl kaitnyl requested review from westonruter and ThierryA Jan 11, 2018

@kaitnyl kaitnyl self-assigned this Jan 11, 2018

@kaitnyl kaitnyl changed the title WIP: #849 Add paired mode custom templates #849 Add paired mode custom templates Jan 11, 2018

$this->template_dir = apply_filters( 'amp_post_template_dir', AMP__DIR__ . '/templates' );
if ( isset( $args['active_callback'] ) ) {
$this->active = $args['active_callback']();

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 12, 2018

Member

I think this may need to be call_user_func( $args['active_callback'] ) for PHP 5.2.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 12, 2018

Member

Nevermind! I was way wrong. This works even back in PHP 4: https://3v4l.org/TcVa0

@westonruter

This comment has been minimized.

Copy link
Member

commented Jan 12, 2018

@kaitnyl It looks like I pushed up some code after you pushed up yours. Take a look at this line of my #857 PR here:

https://github.com/Automattic/amp-wp/blob/20838af95a8445f01a3a578ada9d53543aed3761/amp.php#L129-L146

I think that instead of adding code to AMP_Post_Template we can instead make a clean break and instead add your code to a new AMP_Paired_Mode_Actions. This would make sense I think because in our new paired mode system, it's not limited to posts at all, which the existing templating system is limited to. This would then answer your question about amp_prepare_render(), because with the PR #857 this never gets called at all when a theme supports amp.

@westonruter

This comment has been minimized.

Copy link
Member

commented Jan 13, 2018

@kaitnyl The size of this PR increased due to the merge of both #857 and #858.

$args = array_shift( $support );
// @todo We might want to rename active_callback to available_callback..
if ( isset( $args['active_callback'] ) && is_callable( $args['active_callback'] ) ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 13, 2018

Member

Should we rename active_callback to available_callback? It would seem a little strange for an active callback to return true if on the frontend in paired mode, since is_amp_endpoint() would be false.

This comment has been minimized.

Copy link
@kaitnyl

kaitnyl Jan 15, 2018

Author Contributor

I agree the name isn't the most accurate. I've updated it to available_callback in b10c0fe, but display_callback might even be a more clear. Thoughts?

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 15, 2018

Member

I think I like available more. It sounds more like something that would return a boolean. There is a display_callback in core, but it is used in widgets to actually render something out. Otherwise, in core, there is active_callback and video-active-callback.

Core usage: https://gist.github.com/westonruter/49552860d1e57a20547bcebd090ef269

I'm torn between available and active.

if ( isset( $args['template_path'] ) ) {
$amp_templates = array();
foreach ( $templates as $template ) {
$amp_templates[] = $args['template_path'] . '/' . $template;

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 13, 2018

Member

Since template_path is a relative path from the theme root to locate the AMP templates directory, is this still the best name?

This comment has been minimized.

Copy link
@kaitnyl

kaitnyl Jan 15, 2018

Author Contributor

I've updated these to use template_dir which is what AMP_Post_Template is calling it, too. Setting this will look like the following, which should be easier since it'll be done inside of a theme:

add_theme_support( 'amp', array(
	'template_dir' => 'amp-templates',

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 15, 2018

Member

Right, there is the amp_post_template_dir filter:

$this->template_dir = apply_filters( 'amp_post_template_dir', AMP__DIR__ . '/templates' );

One difference is that the template_dir is absolute here, whereas what we're doing in the theme support is relative. I doubt there will be confusion anyway.

kaitnyl added some commits Jan 15, 2018

Update name template_path to template_dir when it comes to custom pai…
…red mode templates to match AMP_Post_Template naming.
@ThierryA
Copy link
Collaborator

left a comment

@westonruter @kaitnyl below is my CR. Thanks,

@@ -14,6 +14,84 @@ class AMP_Theme_Support {
const COMPONENT_SCRIPTS_PLACEHOLDER = '<!--AMP_COMPONENT_SCRIPTS_PLACEHOLDER-->';

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 15, 2018

Collaborator

Add PHPDoc.

amp.php Outdated
@@ -131,9 +131,9 @@ function amp_maybe_add_actions() {
// Add hooks for when a themes that support AMP.
if ( current_theme_supports( 'amp' ) ) {
if ( amp_is_canonical() || is_amp_endpoint() ) {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 15, 2018

Collaborator

is_amp_endpoint() returns true if amp_is_canonical() so this if statement could simply be if ( is_amp_endpoint() ).

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 15, 2018

Collaborator

Since $is_amp_endpoint = is_amp_endpoint(); is declared further down, I would suggest to move it up to the top of this function and replace is_amp_endpoint() with the variable.

amp.php Outdated
} else {
AMP_Frontend_Actions::register_hooks();
amp_add_frontend_actions();

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 15, 2018

Collaborator

Why calling this here again since it is already a wp_head callback and it already has the necessary conditional statement in the function itself?

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 15, 2018

Member

@ThierryA I don't understand. This function is only being called here in the amp_maybe_add_actions() function. It gets called here at the top in the case where the theme supports amp. Otherwise, it gets called below when the theme does not.

$args = array_shift( $support );
if ( isset( $args['available_callback'] ) && is_callable( $args['available_callback'] ) ) {
return $args['available_callback']();

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 15, 2018

Collaborator

Using call_user_func() would probably be more appropriate here.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 15, 2018

Member

I suppose it would be more readable.

return;
}
$amp_url = amp_get_permalink( get_queried_object_id() );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 15, 2018

Collaborator

Not really a need to store this in a variable, the following would be readable too:

printf(
	'<link rel="amphtml" href="%s">',
	esc_url( amp_get_permalink( get_queried_object_id() ) )
);
public static function init() {
require_once AMP__DIR__ . '/includes/amp-post-template-actions.php';
if ( amp_is_canonical() ) {
$is_amp_endpoint = ( false !== get_query_var( AMP_QUERY_VAR, false ) ); // Because is_amp_endpoint() now returns true if amp_is_canonical().

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 15, 2018

Collaborator

There isn't really a need to store this in a variable, the condition could be added directly in the if statement below.

*/
public static function register_paired_hooks() {
foreach ( self::$template_types as $template_type ) {
add_filter( "{$template_type}_template_hierarchy", array( __CLASS__, 'filter_paired_template_hierarchy' ) );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 15, 2018

Member

It would be preferred if there was a template_hierarchy filter in core so that the list of types wouldn't have to be hard-coded. But alas, there is no such filter: https://github.com/WordPress/wordpress-develop/blob/fb5b6170564b403944a8fbf8a4e6eb877b26a092/src/wp-includes/template.php#L30-L44

@westonruter westonruter merged commit a4c8f36 into develop Jan 15, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/849-paired-mode-templates branch Jan 15, 2018

@westonruter westonruter added this to the v0.7 milestone Jan 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.