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 mox.tv ad network integration for amp-ad #20076

Merged
merged 5 commits into from Jan 9, 2019
Merged

Conversation

llays
Copy link
Contributor

@llays llays commented Dec 26, 2018

No description provided.

Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

Thanks for adding your vendor to AMP! 😄

Just a small question on the config, and also, one last request I would like to make:

Can you add your vendor to: extensions/amp-ad/amp-ad.md, it's sorted alphabetically so add yourself in the appropriate place.

Thank you very much again! 😄 👍

ads/_config.js Outdated
@@ -642,6 +642,8 @@ export const adConfig = {
renderStartImplemented: true,
},

'mox': {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello! Just wanted to make sure, your servers don't support renderStart, or prefetch? From my understanding this improves performance on our end.

cc @zhouyx

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello!
I've added some improvements to config but integration tests has failed.
I didn't understand an error explanation and don't know how to fix it ☹️

@torch2424
Copy link
Contributor

torch2424 commented Dec 27, 2018

@llays

Hello! So the integration test was a flaky test on our end, so no worries! 😄

However, I went ahead and tested the ad manually, and got the following:

Testing examples/ads.amp.html
screen shot 2018-12-27 at 11 56 49 am

Testing render start in amp-ad-xorigin-iframe-handler.js
screen shot 2018-12-27 at 11 57 12 am

So first, I noticed the ad is only 1px tall, so we cannot see the ad, without manually making it larger in the example. Also, we noticed renderStart is never being called. I tried this in mobile device emulation, so I am not sure if it is something wrong with the server, or the AMP implementation. 🤔 Either way, let me know how we can resolve these issues, and I think it will be good to go! 👍

Thank you!

@llays
Copy link
Contributor Author

llays commented Dec 28, 2018

Hi!
It's our test ad campaign with 100% fill rate, but it looks like our targeting filters aren't configured well.
Can you please recheck ad using VPN from other country and/or select another device in emulation instead of iPhone 5/SE?
Thank you!

@torch2424
Copy link
Contributor

@llays Hello!

So I tried on multiple different device emulations in the dev tools, two real life devices (Pixel 2 Android, iPhone 6 SE), and through a Candian VPN I have set up. And the ad was still not getting filled on my end unfortunately 😢

Is everything working on your end when you serve examples/ads.amp.html?

@llays
Copy link
Contributor Author

llays commented Jan 9, 2019

@torch2424 Hello!

I have great news, we've found a bug due to which you had no ad. And we've already fixed it! Could you please recheck again?

Is everything working on your end when you serve examples/ads.amp.html?

Yes, I check it before every push.
image

Thank you!

Copy link
Contributor

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

LGTM 😄

screen shot 2019-01-09 at 12 36 20 pm

@torch2424
Copy link
Contributor

@llays

Thank you for fixing that! Also, thank you for checking the example before pushing, helps us a lot 😄

Going to merge this in, thank you very much for your contribution! 👍

@torch2424 torch2424 merged commit 6d8eb87 into ampproject:master Jan 9, 2019
@llays
Copy link
Contributor Author

llays commented Jan 10, 2019

@torch2424

Thank you very much for participating! 🙌👍

noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* Add mox.tv ad network integration for amp-ad

* Added more efficient config

* Added mox.tv to supported ad networks list

* Remove useless preconnect
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
New Ads Vendor PR
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants