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.

Sign up for GitHub

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

Jump to bottom

#849 Add paired mode custom templates #856

Merged
merged 19 commits into from
Jan 15, 2018
Merged

#849 Add paired mode custom templates #856

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
57926fb
Bumped plugin version to 0.6-beta
ThierryA Dec 20, 2017
309af3d
Update version and readme
westonruter Jan 9, 2018
ed5594b
Merge pull request #853 from eshannon3/eshannon3/fix-header-id to 0.6…
westonruter Jan 10, 2018
63dae3e
Allow custom AMP template folder to be specified from a theme.
kaitnyl Jan 11, 2018
2bb0bb4
Move to filtering template_include when it comes to custom templates …
kaitnyl Jan 11, 2018
6bbe965
Callback to further control when custom paired mode templates should …
kaitnyl Jan 11, 2018
ca4536c
Fix default if no callback given.
kaitnyl Jan 11, 2018
1c5878d
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter Jan 12, 2018
95af9e7
Move paired mode template loading into AMP_Theme_Support class
westonruter Jan 13, 2018
184e89a
Ensure post_supports_amp() and is_amp_endpoint() return true when can…
westonruter Jan 13, 2018
13bbfab
Revert "Merge pull request #725 from Automattic/amedina/add-amp-actio…
westonruter Jan 13, 2018
a378cd5
Merge branch 'revert/amp-actions-abstraction' (#858) into add/849-pai…
westonruter Jan 13, 2018
624cdb6
Redirect from AMP endpoint to canonical when AMP is canonical
westonruter Jan 13, 2018
2af460e
Re-use amp_add_generator_metadata for AMP theme support
westonruter Jan 13, 2018
e5b5490
Prevent showing amphtml link if theme supports AMP but paired mode is…
westonruter Jan 13, 2018
6285f9f
Update name template_path to template_dir when it comes to custom pai…
kaitnyl Jan 15, 2018
b10c0fe
Address callback param name.
kaitnyl Jan 15, 2018
9b1828d
Update test_amp_is_canonical() with new naming.
kaitnyl Jan 15, 2018
34c18cc
Cleanup: removing unnecessary variables, add PHPDoc for new const, re…
kaitnyl Jan 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions amp.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() ) {
Copy link
Collaborator

@ThierryA ThierryA Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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_Theme_Support::register_hooks();
AMP_Theme_Support::init();
} else {
AMP_Frontend_Actions::register_hooks();
amp_add_frontend_actions();
Copy link
Collaborator

@ThierryA ThierryA Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@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.

}
return;
}
Expand All @@ -152,7 +152,7 @@ function amp_maybe_add_actions() {
$supports = post_supports_amp( $post );

if ( ! $supports ) {
if ( $is_amp_endpoint ) {
if ( $is_amp_endpoint && isset( $post->ID ) ) {
wp_safe_redirect( get_permalink( $post->ID ), 301 );
exit;
}
Expand Down Expand Up @@ -196,12 +196,13 @@ function amp_load_classes() {
}

function amp_add_frontend_actions() {
AMP_Frontend_Actions::register_hooks();
require_once AMP__DIR__ . '/includes/amp-frontend-actions.php';
}

function amp_add_post_template_actions() {
AMP_Paired_Post_Actions::register_hooks();
require_once( AMP__DIR__ . '/includes/amp-post-template-functions.php' );
require_once AMP__DIR__ . '/includes/amp-post-template-actions.php';
require_once AMP__DIR__ . '/includes/amp-post-template-functions.php';
amp_post_template_init_hooks();
}

function amp_prepare_render() {
Expand Down
20 changes: 0 additions & 20 deletions includes/actions/class-amp-actions.php
View file
Open in desktop

This file was deleted.

63 changes: 0 additions & 63 deletions includes/actions/class-amp-frontend-actions.php
View file
Open in desktop

This file was deleted.

176 changes: 0 additions & 176 deletions includes/actions/class-amp-paired-post-actions.php
View file
Open in desktop

This file was deleted.

33 changes: 33 additions & 0 deletions includes/amp-frontend-actions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php
/**
* Callbacks for adding AMP-related things to the main theme.
*
* @package AMP
*/

add_action( 'wp_head', 'amp_frontend_add_canonical' );

/**
* Add amphtml link to frontend.
*
* @since 0.2
*/
function amp_frontend_add_canonical() {

// Prevent showing amphtml link if theme supports AMP but paired mode is not available.
if ( current_theme_supports( 'amp' ) && ! AMP_Theme_Support::is_paired_available() ) {
return;
}

/**
* Filters whether to show the amphtml link on the frontend.
*
* @since 0.2
*/
if ( false === apply_filters( 'amp_frontend_show_canonical', true ) ) {
return;
}

$amp_url = amp_get_permalink( get_queried_object_id() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

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() ) )
);

printf( '<link rel="amphtml" href="%s">', esc_url( $amp_url ) );
}
8 changes: 8 additions & 0 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ function amp_get_permalink( $post_id ) {
* @return bool Whether the post supports AMP.
*/
function post_supports_amp( $post ) {
if ( amp_is_canonical() ) {
return true;
}

return 0 === count( AMP_Post_Type_Support::get_support_errors( $post ) );
}

Expand All @@ -74,6 +78,10 @@ function post_supports_amp( $post ) {
* @return bool Whether it is the AMP endpoint.
*/
function is_amp_endpoint() {
if ( amp_is_canonical() ) {
return true;
}

if ( 0 === did_action( 'parse_query' ) ) {
_doing_it_wrong( __FUNCTION__, sprintf( esc_html__( "is_amp_endpoint() was called before the 'parse_query' hook was called. This function will always return 'false' before the 'parse_query' hook is called.", 'amp' ) ), '0.4.2' );
}
Expand Down
Loading