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

feat(gam): support fluid ad unit sizing #196

Merged
merged 13 commits into from
Oct 19, 2021
Merged

Conversation

miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Oct 8, 2021

Implements support for GAM's fluid ad unit size. Fluid is supported multisized or not for both AMP and AMP Plus.

00cee00 added a fix to fluid rendering on AMP Plus. GPT currently sets the iframe with min-width set to 100% and width attribute set to 0. This causes the iframe to be rendered with 0 width. The fix listens to rendered slot and updates the width if native content is identified.

How to test changes

  1. Check out this branch, feat(ads): fluid ad unit sizing newspack-plugin#1181, and create 4 new ad units:
    1. In legacy mode and with uploaded credentials:
      1. With a regular fixed size and fluid option checked
      2. Without any size and fluid option checked
  2. Make sure you have an active GAM campaign to display fluid and the selected fixed size
  3. Add the ad units to a page
  4. With AMP Plus disabled:
    1. Make sure the multisized ad units are properly displayed and rotate between the fixed and fluid content
    2. Make sure the fluid-only ad unit is properly displayed
  5. With AMP Plus enabled, refresh the page and make sure it behaves the same

Note: There's a known issue with AMP that sometimes won't render ads that are loaded on the viewport. Make sure to put the ads outside of the initial viewport and will require you to scroll to the ad for it to properly load.

Closes #195

@miguelpeixe miguelpeixe requested a review from a team October 14, 2021 17:58
@miguelpeixe miguelpeixe self-assigned this Oct 14, 2021
@miguelpeixe miguelpeixe marked this pull request as ready for review October 14, 2021 17:58
includes/class-newspack-ads-blocks.php Outdated Show resolved Hide resolved
@miguelpeixe
Copy link
Member Author

Thank you, @adekbadek ! Do you mind taking another quick look? I had to merge #202

@miguelpeixe miguelpeixe merged commit cf80344 into master Oct 19, 2021
@miguelpeixe miguelpeixe deleted the feat/fluid-ad-unit branch October 19, 2021 14:06
matticbot pushed a commit that referenced this pull request Oct 19, 2021
# [1.18.0-alpha.1](v1.17.1...v1.18.0-alpha.1) (2021-10-19)

### Features

* **gam:** support fluid ad unit sizing ([#196](#196)) ([cf80344](cf80344))

### Performance Improvements

* cache ad units ([#202](#202)) ([e36d2cb](e36d2cb))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.18.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Oct 19, 2021
# [1.18.0](v1.17.1...v1.18.0) (2021-10-19)

### Features

* **gam:** oauth connection ([#203](#203)) ([ad5a5ac](ad5a5ac))
* **gam:** support fluid ad unit sizing ([#196](#196)) ([cf80344](cf80344))

### Performance Improvements

* cache ad units ([#202](#202)) ([e36d2cb](e36d2cb))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.18.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 'fluid' size option
3 participants