Add new filter for amp-story-auto-ads #2492
Conversation
| @@ -62,12 +62,17 @@ | |||
| poster-landscape-src="<?php echo esc_url( $poster_landscape ); ?>" | |||
| <?php endif; ?> | |||
| > | |||
| <?php the_content(); ?> | |||
| <?php | |||
| amp_print_story_auto_ads(); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
See #2430.