Integrating Ad.Style to AMP #24264
Integrating Ad.Style to AMP #24264
Conversation
ZagiMo
force-pushed
the
adstyle-integration
branch
from
5d060d4
to
3b23856
Compare
|
Friendly ping for a review. Thank you! |
|
@leonidvolinski Could you also add your network to |
ads/_config.js
Outdated
| @@ -193,6 +193,8 @@ const adConfig = jsonConfiguration({ | |||
| preconnect: 'https://ad.ad-stir.com', | |||
| }, | |||
|
|
|||
| 'adstyle': {}, | |||
There was a problem hiding this comment.
I see your not using renderStart here, we recommend implementing it if possible, should help with UX and get some performance gains. It is optional.
extensions/amp-ad/amp-ad.md
Outdated
| @@ -271,6 +271,7 @@ See [amp-ad rules](https://github.com/ampproject/amphtml/blob/master/extensions/ | |||
| - [Aniview](../../ads/aniview.md) | |||
| - [AppNexus](../../ads/appnexus.md) | |||
| - [AppVador](../../ads/appvador.md) | |||
| - [AdStyle](../../ads/adstyle.md) | |||
There was a problem hiding this comment.
nit: please sort alphabetically.
ads/_config.js
Outdated
| @@ -193,6 +193,12 @@ const adConfig = jsonConfiguration({ | |||
| preconnect: 'https://ad.ad-stir.com', | |||
| }, | |||
|
|
|||
| 'adstyle': { | |||
| renderStartImplemented: true, | |||
There was a problem hiding this comment.
Great that you decided to implement this, but I do not see this callback being executed. Can you confirm you are calling this in you external JS? If you are unable to do so you should remove this flag (keep the prefetch and preconnect) as you will suffer a small penalty if the runtime is waiting for this signal but it is never called.
This is the integration of Ad.Style to AMP.
Contributor License Agreement already signed.