[DRAFT] <amp-embed type="insticator"> #23928

vladdeev · 2019-08-13T19:37:27Z

This is a draft implementation of <amp-embed type=“insticator”>.
The intended functionality is described in #23257.
We need technical advice before going further.

Implementation descriptionю
The <amp-embed> creates an iframe with a boilerplate document inside.
We add three children to the div#c in that document in the following order:
*amp-ad which holds the upper ad
*iframe which holds our embed
*amp-ad which holds the lower ad

cc @lannka

…l-setup draft implementation of amp-embed type insticator

googlebot · 2019-08-13T19:37:33Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. 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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

lannka · 2019-08-14T01:39:26Z

ads/insticator.js

+};
+
+// ------- HELPER FUNCTIONS ------- //
+function createInitialMarkup() {


for flexibility in a long run, I suggest you only leave config related logic in this file, and move logic to /amp-embed-lib/insticator.js as much as possible.

ortemcheg · 2019-08-14T15:15:00Z

Currently the ads won’t render because the boilerplate HTML they get appended to is not a valid AMP.
One way to make it work is to use iframes with valid AMP in place of <amp-ad>s. If we take that path we’ll end up with three iframes placed inside of an iframe created by <amp-embed>. We think such an implementation would be ugly, slow and inefficient, so we need to implement it differently. So now we badly need suggestions from the AMP team to help us with implementation. @lannka, could you help?

Because the only alternative way of implementing this functionality that we see now is to consider once again <amp-insticator>, which doesn’t have any nested iframes and uses native <amp-ads> organically.

triso07 · 2019-08-16T15:08:29Z

@lannka Any help you can provide on the above would be great. We're starting to get under the gun in terms of our build effort here. Need to start making consistent forward progress now.

triso07 · 2019-08-16T15:10:22Z

@lannka Maybe we can just hop on a call if you're open to that? That may speed up the process and make this easier for all of us.

triso07 · 2019-08-19T14:10:24Z

@lannka Please advise on the best course of action here

triso07 · 2019-08-21T19:38:16Z

@lannka Can we just get on a call to hash this out? We can probably clear this up very quickly and move forward. This starting and stopping of development is hurting our progress and we have very real business needs we're trying to meet here.

triso07 · 2019-08-21T19:42:20Z

This back and forth messaging feels a bit slow and unproductive. A call would help expedite this process. We have technical concerns with <amp-embed>. Unless we can resolve those <amp-insticator> seems a much more viable implementation.

lannka · 2019-08-21T20:23:27Z

sorry for the delay. VC sounds good. Are you in GMT+3? I'm in PST. Try to find a good time for both.

lannka · 2019-08-21T20:24:49Z

BTW, inside the iframe, why do you still need amp-ad? You can have your own ads tag to retrieve ads. For sake of performance, you might consider server-to-server fetch to reduce client side redirect.

triso07 · 2019-08-21T20:27:16Z

Thanks for the response. We're EST. If you have time tomorrow that would be great. How's 4:30pm EST?

lannka · 2019-08-22T20:02:54Z

chatted over VC, meeting notes:

<amp-embed> uses 3rd party iframe, inside the iframe Instricator can do things just like non-AMP pages, with some limitation as noted below
To prevent page jump, like any other AMP extension, <amp-embed> is not allowed to resize itself freely. Instead, 3rd party code should call the context API to do resize. The context API also provides a bunch of commonly used API for 3rd party code to retrieve relevant info about the apge.
The code snippet that Instricator provides to their customers should look like: <amp-embed type=instricator width=300 height=750>. That said, a predefined size need to be specified.
Elements within current viewport are generally not allowed for resize. To increase success rate of a resize, do it as early as possible before user scroll to it.
On a resize failure, Instricator can consider to use an overflow button to do a user initiated resize.

…of AMP ads)

…ads the traditional way

Utilizing existing header/body code setup to generate embed and ads

- Cleaned up unnecessary code references - Removed unnecessary preconnects - Updated documentation slightly

Dynamically pulling data attributes and fully cleaned up code

…Updated documentation slightly.

…r ads code for passing to providers

Passing top level domain to insticator ads code

Sandbox

vladdeev · 2019-10-07T18:54:15Z

Also it seems like there are now a few commits with authors that are not covered by the Google CLA, this will have to be resolved before we can merge.

hey @calebcordry I added these 2 addresses to the group. Do we need to do anything else to pass this check?

googlebot · 2019-10-08T13:50:50Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

calebcordry · 2019-10-08T13:53:18Z

Thanks for looking into the CLA. I do see the new emails passing but it looks like a previously ok email is now failing.

triso07 · 2019-10-08T14:57:02Z

@googlebot I consent.

googlebot · 2019-10-08T14:57:47Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2019-10-08T14:57:47Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

triso07 · 2019-10-08T14:59:47Z

@calebcordry CLAs signed. We good to go?

calebcordry · 2019-10-08T17:04:39Z

ads/insticator.js

+  validateData(data, ['siteId', 'embedId']);
+
+  // create insticator markup
+  global.document.getElementById('c').appendChild(createTemplate(data['embedId']));


nit: linter wants these on new lines. If you run gulp lint it will show these errors.

Another NIT linter issue

triso07 · 2019-10-09T16:12:12Z

@calebcordry Great! Thanks! Excited!

rcebulko · 2019-10-09T16:22:36Z

Oddly, requesting the set of reviews from the GitHub API for this PR yields the following series of states:

[ [ 'COMMENTED', 'lannka' ],
  [ 'DISMISSED', 'lannka' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'lannka' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'calebcordry' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'triso07' ],
  [ 'COMMENTED', 'calebcordry' ] ]

Investigating why GitHub thinks you haven't approved @calebcordry

calebcordry · 2019-10-09T16:36:19Z

@rcebulko Thanks for investigating.

@triso07 You are welcome. Thanks for being patient on this contribution! FYI we are investigating what is going on with our owners bot, no further changes are needed from you. I will merge when it gets sorted.

triso07 · 2019-10-17T16:00:30Z

@calebcordry @rcebulko Had to make some minor updates to the .md read me file. Sorry for that. Anyway we can review those and merge?

Insticator#26

calebcordry · 2019-10-17T16:18:02Z

@triso07 do you have an open PR?

triso07 · 2019-10-18T14:34:46Z

@calebcordry We do now ;)

#25103

* UNSTABLE: added new amp-embed type, insticator * Utilizing our standard header and body code to generate ads (instead of AMP ads) * Commenting out extraneous helper functions now that we're generating ads the traditional way * commented out logs * - Added site id param - Cleaned up unnecessary code references - Removed unnecessary preconnects - Updated documentation slightly * further cleaning the code * Changed append location of markup to be more inline with other code. Updated documentation slightly. * minor text change to documentation * hooking up to available API and passing top level domain to insticator ads code for passing to providers * Passing AMP embed API to insticator code so we can resize when needed * initial round of AMP requested adjustments to our code * No longer creating script tags then appending them, instead just executing the functions * removed unnecessary function * removed extraneous helper function * changed var to const * Removing unnecessary library call. Using dummy embed instead of real one. * Implemented renderStart method per AMP request * Fixed all reported linting issues * Embed was not loading correctly before. Using a to reference Insticator object so we pass linter and embed works correctly * Updated Copyright year * removed loadScript (thought this was necessary from AMPs side) * removed import for loadScript since no longer using it * added /*OK*/ to template as requested * Removed URL helper var since no longer being used in multiple places. Instead hardcoding URL in string. * Using /*OK*/ for innerHTML as AMP requires this * Starting at 850 as that is our standard for embed with ads (resize will be requested if necessary) * Capitalized E in element * using bracket notation to reference data attributes per AMP request * Additional bracket instead of dot notation reference * forgot the tick marks when accessing property via brackets * Another NIT linter issue

ortemcheg and others added 4 commits August 2, 2019 22:22

UNSTABLE: added new amp-embed type, insticator

ce8a869

Merged in updates

5360271

Merge branch 'master' into amp-embed-type-insticaor-initial-setup

39a0491

Merge pull request #5 from Insticator/amp-embed-type-insticaor-initia…

6384f77

…l-setup draft implementation of amp-embed type insticator

googlebot added the cla: no label Aug 13, 2019

lannka self-requested a review August 13, 2019 19:38

lannka reviewed Aug 14, 2019

View reviewed changes

triso07 and others added 12 commits August 28, 2019 11:23

Utilizing our standard header and body code to generate ads (instead …

4b12980

…of AMP ads)

Commenting out extraneous helper functions now that we're generating …

f154c66

…ads the traditional way

commented out logs

285ec22

Merge pull request #6 from Insticator/sandbox

793d332

Utilizing existing header/body code setup to generate embed and ads

- Added site id param

732d146

- Cleaned up unnecessary code references - Removed unnecessary preconnects - Updated documentation slightly

further cleaning the code

e8ff916

Merge pull request #7 from Insticator/sandbox

232585e

Dynamically pulling data attributes and fully cleaned up code

Changed append location of markup to be more inline with other code. …

6bf457a

…Updated documentation slightly.

minor text change to documentation

bef3245

hooking up to available API and passing top level domain to insticato…

50d46fb

…r ads code for passing to providers

Merge pull request #8 from Insticator/sandbox

461cba4

Passing top level domain to insticator ads code

Passing AMP embed API to insticator code so we can resize when needed

a94add8

triso07 and others added 4 commits October 4, 2019 13:44

using bracket notation to reference data attributes per AMP request

fa492de

Additional bracket instead of dot notation reference

feb6772

forgot the tick marks when accessing property via brackets

eeb8e18

Merge pull request #24 from Insticator/sandbox

10ae65b

Sandbox

googlebot added cla: yes and removed cla: no labels Oct 8, 2019

calebcordry reviewed Oct 8, 2019

View reviewed changes

triso07 and others added 2 commits October 9, 2019 10:46

Another NIT linter issue

11edc2f

Merge pull request #25 from Insticator/sandbox

8eed75d

Another NIT linter issue

calebcordry approved these changes Oct 9, 2019

View reviewed changes

rcebulko self-requested a review October 9, 2019 16:07

rcebulko mentioned this pull request Oct 9, 2019

[owners] Bugfix: Auto-page reviews from GitHub API ampproject/amp-github-apps#528

Merged

rcebulko removed their request for review October 9, 2019 17:11

rcebulko approved these changes Oct 9, 2019

View reviewed changes

calebcordry merged commit cfed375 into ampproject:master Oct 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] <amp-embed type="insticator"> #23928

[DRAFT] <amp-embed type="insticator"> #23928

vladdeev commented Aug 13, 2019

googlebot commented Aug 13, 2019

lannka Aug 14, 2019

ortemcheg commented Aug 14, 2019

triso07 commented Aug 16, 2019

triso07 commented Aug 16, 2019

triso07 commented Aug 19, 2019

triso07 commented Aug 21, 2019

triso07 commented Aug 21, 2019

lannka commented Aug 21, 2019

lannka commented Aug 21, 2019

triso07 commented Aug 21, 2019

lannka commented Aug 22, 2019 •

edited

vladdeev commented Oct 7, 2019

googlebot commented Oct 8, 2019

calebcordry commented Oct 8, 2019

triso07 commented Oct 8, 2019 •

edited

googlebot commented Oct 8, 2019

googlebot commented Oct 8, 2019

triso07 commented Oct 8, 2019

calebcordry Oct 8, 2019

triso07 Oct 9, 2019

triso07 commented Oct 9, 2019 •

edited

rcebulko commented Oct 9, 2019

calebcordry commented Oct 9, 2019

triso07 commented Oct 17, 2019 •

edited

calebcordry commented Oct 17, 2019

triso07 commented Oct 18, 2019

[DRAFT] <amp-embed type="insticator"> #23928

[DRAFT] <amp-embed type="insticator"> #23928

Conversation

vladdeev commented Aug 13, 2019

googlebot commented Aug 13, 2019

lannka Aug 14, 2019

Choose a reason for hiding this comment

ortemcheg commented Aug 14, 2019

triso07 commented Aug 16, 2019

triso07 commented Aug 16, 2019

triso07 commented Aug 19, 2019

triso07 commented Aug 21, 2019

triso07 commented Aug 21, 2019

lannka commented Aug 21, 2019

lannka commented Aug 21, 2019

triso07 commented Aug 21, 2019

lannka commented Aug 22, 2019 • edited

vladdeev commented Oct 7, 2019

googlebot commented Oct 8, 2019

calebcordry commented Oct 8, 2019

triso07 commented Oct 8, 2019 • edited

googlebot commented Oct 8, 2019

googlebot commented Oct 8, 2019

triso07 commented Oct 8, 2019

calebcordry Oct 8, 2019

Choose a reason for hiding this comment

triso07 Oct 9, 2019

Choose a reason for hiding this comment

triso07 commented Oct 9, 2019 • edited

rcebulko commented Oct 9, 2019

calebcordry commented Oct 9, 2019

triso07 commented Oct 17, 2019 • edited

calebcordry commented Oct 17, 2019

triso07 commented Oct 18, 2019

lannka commented Aug 22, 2019 •

edited

triso07 commented Oct 8, 2019 •

edited

triso07 commented Oct 9, 2019 •

edited

triso07 commented Oct 17, 2019 •

edited