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

Add new filter for amp-story-auto-ads #2492

Merged
merged 4 commits into from
Jun 3, 2019
Merged

Add new filter for amp-story-auto-ads #2492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
67e794a
Print amp analytics in single story template
swissspidy Jun 3, 2019
1aa924d
Add new amp_print_story_auto_ads function
swissspidy Jun 3, 2019
7640bd3
CS: Add missing trailing comma in test
swissspidy Jun 3, 2019
59edcc3
Use singular noun for filter name
swissspidy Jun 3, 2019
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
31 changes: 31 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 @@ -1130,3 +1130,34 @@ function amp_add_admin_bar_view_link( $wp_admin_bar ) {

$wp_admin_bar->add_menu( $parent );
}

/**
* Prints AMP Stories auto ads.
*
* @since 1.2
*/
function amp_print_story_auto_ads() {
/**
* Filters the configuration data for <amp-story-auto-ads>.
*
* This allows Dynamically inserting ads into a story.
*
* @param array $data Story ads configuration data.
* @param WP_Post $post The current story's post object.
*/
$data = apply_filters( 'amp_story_auto_ads_configuration', array(), get_post() );

if ( empty( $data ) ) {
return;
}

$script_element = AMP_HTML_Utils::build_tag(
'script',
array(
'type' => 'application/json',
),
wp_json_encode( $data )
);

echo AMP_HTML_Utils::build_tag( 'amp-story-auto-ads', array(), $script_element ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
}
7 changes: 6 additions & 1 deletion includes/templates/single-amp_story.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,17 @@
poster-landscape-src="<?php echo esc_url( $poster_landscape ); ?>"
<?php endif; ?>
>
<?php the_content(); ?>
<?php
amp_print_story_auto_ads();
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider injecting the story auto-ads via a the_content filter? I don't know if it is particularly better to use a filter, but it came to mind.

Also, something else which may need to get filtered is the injection of a next-page-no-ad attribute on the amp-story-page components: https://github.com/ampproject/amphtml/blob/master/extensions/amp-story-auto-ads/amp-story-auto-ads.md#insertion-control

I suppose the normal render_block filter should be used for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did you consider injecting the story auto-ads via a the_content filter?

Not really. I figured there'd be too many situations where this wouldn't need to be printed (feeds, embeds, archives, etc.). So this was more straightforward.

However, I don't have a strong opinion on what's better here.

Also, something else which may need to get filtered is the injection of a next-page-no-ad attribute on the amp-story-page components:

Yeah I mentioned that attribute in #2430 (comment). A render_block filter for that sounds a bit hacky but probably the easiest way for a plugin to change the markup without causing block invalidation.

However, there would still need to be some UI to choose the pages where this should be added. So a plugin would use the blocks.registerBlockType filter to add a new attribute for this feature and expose a UI toggle.

Alternatively, we just add this attribute to the page block ourselves and save the block with the proper HTML attribute if it's set. However, there wouldn't be a UI by default. A plugin would be responsible for adding a toggle.

the_content();
?>
</amp-story>

<?php
// Note that \AMP_Story_Post_Type::filter_frontend_print_styles_array() will limit which styles are printed.
print_late_styles();

amp_print_analytics( '' );
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
?>
</body>
</html>
35 changes: 35 additions & 0 deletions tests/test-class-amp-story-post-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,4 +373,39 @@ public function create_story_posts_with_featured_images( $featured_images ) {

return $stories;
}

/**
* Test amp_print_story_auto_ads()
*
* @covers ::amp_print_story_auto_ads()
*/
public function test_amp_print_story_auto_ads_empty() {
$actual = get_echo( 'amp_print_story_auto_ads' );

$this->assertEmpty( $actual );
}

/**
* Test amp_print_story_auto_ads()
*
* @covers ::amp_print_story_auto_ads()
*/
public function test_amp_print_story_auto_ads() {
add_filter(
'amp_story_auto_ads_configuration',
static function() {
return array(
'ad-attributes' => array(
'type' => 'doubleclick',
'data-slot' => '/30497360/a4a/amp_story_dfp_example',
),
);
}
);

$actual = get_echo( 'amp_print_story_auto_ads' );

$this->assertStringStartsWith( '<amp-story-auto-ads', $actual );
$this->assertContains( '<script type="application/json">{"ad-attributes":{"type":"doubleclick"', $actual );
}
}