AMP integration with Holder #6024

AntonOrnatskyi · 2016-11-04T14:00:29Z

Please check the Holder (ukrainian ad network) implementation for amp-ad

I have a Google Individual CLA.
Anton Ornatskyi

Draft w/o lib

…lder

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

…to holder

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

…lder

…to holder

Lint checked Banner received

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

…lder

…to holder

…lder

Draft w/o lib

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

Lint checked Banner received

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

…into holder

googlebot · 2016-11-04T14:00:32Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-11-04T14:35:41Z

CLAs look good, thanks!

AntonOrnatskyi · 2016-11-04T14:39:04Z

CLA completed

erwinmombay · 2016-11-04T20:27:34Z

/to @zhouyx @lannka

lannka

I noticed a slight delay between render-start and real ad rendering. Can you check if the render-start is called at the right place?

lannka · 2016-11-07T17:32:09Z

ads/_config.js


+  ibillboard: {},


nit: keep the blank line after ibillboard

lannka · 2016-11-07T17:33:23Z

ads/holder.js

+export function holder(global, data) {
+  validateData(data, ['block'], ['ampSlotIndex']);
+  const wcl = global.context.location,
+    n = navigator.userAgent;


either end the previous line with ';', or indent 2 more spaces here.

lannka · 2016-11-07T17:33:54Z

ads/holder.js

+  const wcl = global.context.location,
+    n = navigator.userAgent;
+  let l = '&r' + Math.round((Math.random() * 10000000));
+  l += '&' +


merge into the previous line?

lannka · 2016-11-07T17:36:10Z

ads/holder.js

+  l += '&' +
+  'h' + wcl.href;
+  if (!(n.indexOf('Safari') != -1 &&
+  n.indexOf('Chrome') == -1)) {


move to previous line

AntonOrnatskyi · 2016-11-07T20:03:32Z

/to @lannka Sorry, I put render-start as close to end of ad request as possible now.

lannka

I still see a half-second delay between loading indicator and ad render. it is a white blank.

lannka · 2016-11-07T21:49:41Z

ads/holder.js

+ * @param {!Object} data
+ */
+export function holder(global, data) {
+  validateData(data, ['block'], ['ampSlotIndex']);


no need for ampSlotIndex. it's a known bug, you can ignore the harmless console error for now.

AntonOrnatskyi · 2016-11-07T22:35:16Z

I still see a half-second delay between loading indicator and ad render. it is a white blank.

It's best I can achieve for now. :(
For better performance and for noContentAvailable() realisation I have to modify ad-server back-end
and I'll do it in the one of next updates in the near future.

AntonOrnatskyi · 2016-11-08T09:04:05Z

@lannka Is it possible to merge PR 'as is' before I'd update back-end? Or I can remove render-start support until back-end will not support it.

lannka

Sure. That works

AntonOrnatskyi · 2016-11-09T21:57:58Z

Thanks I'll add noContentAvailable() and make render-start faster in the week or two.

* Draft w/o lib Draft w/o lib * Fix * Trying to setup local dev Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * Draft Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * preconnect Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * Dropbox Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * config Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * Worked simple version Lint checked Banner received * holderamp lib v1 Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * First PR candidate * Final, ready to PR * Draft w/o lib Draft w/o lib * Fix * Trying to setup local dev Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * Draft Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * Dropbox Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * config Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * Worked simple version Lint checked Banner received * holderamp lib v1 Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * First PR candidate * Final, ready to PR * Final, ready to PR * Draft w/o lib Final version, ready to PR * Final version, ready for PR * holderamp lib v1 Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua> * PR candidate * Final, ready to PR * Final ready to PR * Cookie request removed * var document - removed * Syntax fixes * ampSlotIndex removed

AntonOrnatskyi and others added 30 commits October 15, 2016 22:51

Merge remote-tracking branch 'refs/remotes/ampproject/master'

3146dba

Merge remote-tracking branch 'refs/remotes/ampproject/master'

66ea111

Draft w/o lib

64194ec

Draft w/o lib

Fix

a527de8

Merge remote-tracking branch 'refs/remotes/ampproject/master' into ho…

ae038fe

…lder

Trying to setup local dev

cbb7b30

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

Merge branch 'holder' of https://github.com/AntonOrnatskyi/amphtml in…

ae9b273

…to holder

Draft

b25387a

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

preconnect

f4cffbc

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

Dropbox

ff4733f

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

config

ac41de5

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

Merge remote-tracking branch 'refs/remotes/ampproject/master' into ho…

53754a9

…lder

Merge branch 'holder' of https://github.com/AntonOrnatskyi/amphtml in…

74dc28f

…to holder

Worked simple version

94e66b3

Lint checked Banner received

holderamp lib v1

5415694

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

Merge remote-tracking branch 'refs/remotes/ampproject/master' into ho…

2acafa0

…lder

Merge branch 'holder' of https://github.com/AntonOrnatskyi/amphtml in…

84e8520

…to holder

First PR candidate

769ea6a

Merge remote-tracking branch 'refs/remotes/ampproject/master' into ho…

5061ba8

…lder

Final, ready to PR

2a3690d

Merge remote-tracking branch 'upstream/master'

85b51b9

Draft w/o lib

a648af0

Draft w/o lib

Fix

6aa6eaa

Trying to setup local dev

18b0e4f

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

Draft

05ef25a

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

Dropbox

cf88cf0

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

config

83f0fa9

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

Worked simple version

d0b6fe5

Lint checked Banner received

holderamp lib v1

9410a16

Signed-off-by: Anton Ornatskyi <a.ornatskyi@digital.umh.ua>

First PR candidate

6618518

Anton Ornatskyi added 2 commits November 4, 2016 15:53

Final ready to PR

9135dfe

Merge branch 'holder' of https://github.com/AntonOrnatskyi/amphtml.git …

50419e3

…into holder

AntonOrnatskyi closed this Nov 4, 2016

Cookie request removed

9550f9a

AntonOrnatskyi reopened this Nov 4, 2016

AntonOrnatskyi closed this Nov 4, 2016

AntonOrnatskyi reopened this Nov 4, 2016

var document - removed

dc5991f

erwinmombay assigned zhouyx and lannka Nov 4, 2016

lannka reviewed Nov 7, 2016

View reviewed changes

AntonOrnatskyi unassigned zhouyx Nov 7, 2016

Syntax fixes

5a6a4a7

AntonOrnatskyi added WG: monetization 3P labels Nov 7, 2016

lannka reviewed Nov 7, 2016

View reviewed changes

ampSlotIndex removed

8f48f63

lannka approved these changes Nov 9, 2016

View reviewed changes

lannka merged commit 67a27d0 into ampproject:master Nov 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AMP integration with Holder #6024

AMP integration with Holder #6024

AntonOrnatskyi commented Nov 4, 2016

googlebot commented Nov 4, 2016

googlebot commented Nov 4, 2016

AntonOrnatskyi commented Nov 4, 2016

erwinmombay commented Nov 4, 2016

lannka left a comment

lannka Nov 7, 2016

AntonOrnatskyi Nov 7, 2016

lannka Nov 7, 2016

AntonOrnatskyi Nov 7, 2016

lannka Nov 7, 2016

AntonOrnatskyi Nov 7, 2016

lannka Nov 7, 2016

AntonOrnatskyi Nov 7, 2016

AntonOrnatskyi commented Nov 7, 2016

lannka left a comment

lannka Nov 7, 2016

AntonOrnatskyi Nov 7, 2016

AntonOrnatskyi commented Nov 7, 2016 •

edited

AntonOrnatskyi commented Nov 8, 2016 •

edited

lannka left a comment

AntonOrnatskyi commented Nov 9, 2016

AMP integration with Holder #6024

AMP integration with Holder #6024

Conversation

AntonOrnatskyi commented Nov 4, 2016

googlebot commented Nov 4, 2016

googlebot commented Nov 4, 2016

AntonOrnatskyi commented Nov 4, 2016

erwinmombay commented Nov 4, 2016

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonOrnatskyi commented Nov 7, 2016

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonOrnatskyi commented Nov 7, 2016 • edited

AntonOrnatskyi commented Nov 8, 2016 • edited

lannka left a comment

Choose a reason for hiding this comment

AntonOrnatskyi commented Nov 9, 2016

AntonOrnatskyi commented Nov 7, 2016 •

edited

AntonOrnatskyi commented Nov 8, 2016 •

edited