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

Add built-in support for Google AMP #53

Open
mboynes opened this issue Sep 20, 2016 · 6 comments
Open

Add built-in support for Google AMP #53

mboynes opened this issue Sep 20, 2016 · 6 comments
Labels
enhancement New feature or request

Comments

@mboynes
Copy link
Contributor

mboynes commented Sep 20, 2016

Ideally, from a developer's perspective, this should be implemented such that the developer would call:

do_action( 'ad_layers_render_amp_ad_unit', $slot );

And the ad would render as an <amp-ad> element. This would probably involve adding a checkbox to the ad layer UI to flag a layer as a "Google AMP Ad Layer" if the AMP plugin is active.

@mboynes mboynes added the enhancement New feature or request label Sep 20, 2016
@davisshaver
Copy link
Contributor

@mboynes Looking into this...

We have is_amp_endpoint() available.

What do you think about this approach?

/**
 * Get the code for a specific ad unit.
 * Should be implemented by all child classes.
 * Since $ad_server will be empty for child classes,
 * this will automatically do nothing if they choose not to implement it.
 *
 * @access public
 */
public function get_ad_unit( $ad_unit, $echo = true ) {
  if ( function_exists( 'is_amp_endpoint' ) && is_amp_endpoint() ) {
    $is_amp = true;
  } else {
    $is_amp = false;
  }
  if ( ! empty( $this->ad_server ) ) {
    return $this->ad_server->get_ad_unit( $ad_unit, $echo, $is_amp );
  }
}

This way a site manager could use an existing sidebar containing ad units on their AMP pages.

@davisshaver
Copy link
Contributor

cc @bcampeau for AMP connection

@mboynes
Copy link
Contributor Author

mboynes commented Mar 1, 2018

@davisshaver I like that approach. In retrospect, I suppose we don't even need the UI changes, do we? Unless, I suppose, someone wanted to serve entirely different ad units on AMP pages, and in that case we'd need targeting options. I'm curious what you think about that.

Thanks!

@davisshaver
Copy link
Contributor

@mboynes That is a great question... More I think about it, I expect we will need something new, but I'm just not sure yet.

If you're cool with the approach I can start testing it out in a branch. My hunch is that the unit size question may force our hand in adding UI elements. As well as the question about whether AMP pages need additional targeting capabilities.

@mboynes
Copy link
Contributor Author

mboynes commented Mar 1, 2018

I'm absolutely on board with the approach, so go right ahead. Thanks for your help and input on this!

@anekola
Copy link

anekola commented May 11, 2022

Is better AMP support on the roadmap still? Specifically looking for the output of ad_unit_js() and targeting_js(), which at the moment are dropped into a <script> tag that is stripped from AMP pages — so in practice, we are unable to target ads on AMP templates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants