Story-Auto-Ads Skeleton #13239

calebcordry · 2018-02-02T19:04:58Z

This is the start of the implementation of a new amp-story-auto-ads component that will handle the scheduling and display of ads in an amp-story.

calebcordry · 2018-02-02T19:05:34Z

@lannka Is there anything else you want in this initial PR?

lannka · 2018-02-02T19:08:32Z

gulpfile.js

@@ -135,6 +135,7 @@ declareExtension('amp-soundcloud', '0.1', {hasCss: false});
 declareExtension('amp-springboard-player', '0.1', {hasCss: false});
 declareExtension('amp-sticky-ad', '1.0', {hasCss: true});
 declareExtension('amp-story', '0.1', {hasCss: true});
+declareExtension('amp-story-auto-ads', '0.1', {hasCss: false});


I think we agreed not to have this as a separate binary. Can you move the code into the amp-story extension?

sure, I guess we can move it later if it bloats

lannka · 2018-02-02T19:15:17Z

extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js

+  // temporary to be replaced with real page later
+  makeMockPage() {
+    const ampStoryAdPage = document.createElement('amp-story-page');
+    ampStoryAdPage.id = 'ad-page-1';


we need to make sure the id not conflict with existing element IDs on the page (provided by the publisher).
you can use our reserved prefix: i-amphtml-

lannka · 2018-02-02T19:16:42Z

extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js

+    const ampStory = this.element.parentElement;
+    user().assert(ampStory.tagName === 'AMP-STORY',
+        `<${TAG}> should be child of <amp-story>`);
+    ampStory.appendChild(this.makeMockPage());


we might want to do this in layoutCallback.

my thought was that we wanted to let the ad resources start loading ASAP but if you want layoutCallback SGTM

we want to be nice to let content render first.
we are not showing ads in the first 3 page views in anyway.

lannka · 2018-02-02T22:23:03Z

extensions/amp-story/0.1/amp-story-auto-ads.js

+  // temporary to be replaced with real page later
+  makeMockPage() {
+    const ampStoryAdPage = document.createElement('amp-story-page');
+    ampStoryAdPage.id = 'i-amphtl-ad-page-1';


typo amphtml.

@newmuis what do you think about this?
i-amphtml- is reserved ID prefix by validator. we can have i-amphtml-ad- as a prefix for all auto generated pages for ads.

this ID will be consumed by analytics.

LGTM. My question is just whether there's something more valuable than a numerical ID that publishers may want for analytics purposes, but I don't understand enough about how publishers consume/evaluate ads to be able to answer that.

that's a good point.

maybe we should include the previous pageId into the ID? If the previous page has an ID of page-3, the ad ID becomes: i-amphtml-AD-page-3

when reporting to analytics, we strip out the i-amphtml- prefix. so publisher will see AD-page-3, so he can infer where the ad is shown.

Of course, we will never show 2 ads next to each other, for sake of user experience, also to make sure no ID conflicts between the ads.

I like this idea, but I think to accomplish it we might need two pieces of identification, one to keep track of it before we know where it will be placed, and another that contains the placement data. Maybe we could use a different attr for one of these?

lannka · 2018-02-02T22:23:32Z

extensions/amp-story/0.1/amp-story.js

@@ -201,7 +201,8 @@ export class AmpStory extends AMP.BaseElement {
  /** @param {!AmpElement} element */
  constructor(element) {
    super(element);
-
+
+    debugger;


debugger? :-)

wow that last commit was sloppy

lannka · 2018-02-02T22:23:45Z

extensions/amp-story/0.1/navigation-state.js

@@ -41,7 +41,7 @@ export class NavigationState {
   * @param {!Element} storyElement
   * @param {function():Promise<boolean>} hasBookend
   */
-  constructor(storyElement, hasBookend) {
+  constructor(storyElement, hasBookend) {]de


newmuis · 2018-02-02T22:20:32Z

extensions/amp-story/0.1/amp-story.js

@@ -201,7 +201,8 @@ export class AmpStory extends AMP.BaseElement {
  /** @param {!AmpElement} element */
  constructor(element) {
    super(element);
-
+
+    debugger;


newmuis · 2018-02-02T22:20:38Z

extensions/amp-story/0.1/navigation-state.js

@@ -41,7 +41,7 @@ export class NavigationState {
   * @param {!Element} storyElement
   * @param {function():Promise<boolean>} hasBookend
   */
-  constructor(storyElement, hasBookend) {
+  constructor(storyElement, hasBookend) {]de


newmuis · 2018-02-02T22:21:35Z

extensions/amp-story/0.1/amp-story-auto-ads.js

+  /** @param {!AmpElement} element */
+  constructor(element) {
+    super(element);
+    this.ampStory = null;


newmuis · 2018-02-02T22:22:12Z

extensions/amp-story/0.1/amp-story-auto-ads.js

+  // temporary to be replaced with real page later
+  makeMockPage() {
+    const ampStoryAdPage = document.createElement('amp-story-page');
+    ampStoryAdPage.id = 'i-amphtl-ad-page-1';


nit: typo (missing "m": i-amphtml-ad-page-1)

newmuis · 2018-02-02T22:26:57Z

extensions/amp-story/0.1/amp-story-auto-ads.js

+
+  /** @override */
+  layoutCallback() {
+    this.ampStory.appendChild(this.makeMockPage());


LGTM for now. I don't think this is going to be sufficient long-term, since the amp-story won't know its children have changed. We'll need to devise a way to tell the story that it has a new page. You could either dispatch an event, or expose a function from AmpStory and call it here, like:

ampStory.getImpl().then(storyImpl => { storyImpl.addNewPage(this.makeMockPage()); });

In long term, maybe worth to extract a PageManager class to decouple the dependency.

cramforce · 2018-02-02T22:33:10Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Syntax error

Comment posted by lgtm.com

* Component skeleton * move files into amp-story * clean up example * remove bad link * bad mock test * clean up

Component skeleton

8eb6071

googlebot added the cla: yes label Feb 2, 2018

calebcordry requested a review from lannka February 2, 2018 19:05

lannka requested changes Feb 2, 2018

View reviewed changes

lannka reviewed Feb 2, 2018

View reviewed changes

calebcordry added 3 commits February 2, 2018 11:43

move files into amp-story

51f660a

clean up example

00c99be

remove bad link

8abec25

lannka added the PR use: Work in Progress label Feb 2, 2018

lannka approved these changes Feb 2, 2018

View reviewed changes

calebcordry removed the PR use: Work in Progress label Feb 2, 2018

lannka requested a review from newmuis February 2, 2018 21:47

bad mock test

2f4e4ae

lannka reviewed Feb 2, 2018

View reviewed changes

newmuis requested changes Feb 2, 2018

View reviewed changes

clean up

08aa422

newmuis approved these changes Feb 5, 2018

View reviewed changes

calebcordry merged commit 0aad6b4 into ampproject:master Feb 5, 2018

calebcordry deleted the story-auto-ad-skeleton branch February 5, 2018 21:26

ampprojectbot added the PR use: In Stable label Feb 14, 2018

protonate pushed a commit to protonate/amphtml that referenced this pull request Feb 26, 2018

Story-Auto-Ads Skeleton (ampproject#13239)

e689c15

* Component skeleton * move files into amp-story * clean up example * remove bad link * bad mock test * clean up

RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018

Story-Auto-Ads Skeleton (ampproject#13239)

6e4074b

* Component skeleton * move files into amp-story * clean up example * remove bad link * bad mock test * clean up

protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018

Story-Auto-Ads Skeleton (ampproject#13239)

2c2f49f

* Component skeleton * move files into amp-story * clean up example * remove bad link * bad mock test * clean up

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Story-Auto-Ads Skeleton #13239

Story-Auto-Ads Skeleton #13239

calebcordry commented Feb 2, 2018

calebcordry commented Feb 2, 2018

lannka Feb 2, 2018

calebcordry Feb 2, 2018

calebcordry Feb 2, 2018

lannka Feb 2, 2018

calebcordry Feb 2, 2018

lannka Feb 2, 2018

calebcordry Feb 2, 2018

calebcordry Feb 2, 2018

lannka Feb 2, 2018

lannka Feb 2, 2018

newmuis Feb 2, 2018

lannka Feb 2, 2018 •

edited

calebcordry Feb 2, 2018

lannka Feb 2, 2018

calebcordry Feb 2, 2018

calebcordry Feb 2, 2018

lannka Feb 2, 2018

calebcordry Feb 2, 2018

newmuis Feb 2, 2018

calebcordry Feb 2, 2018

newmuis Feb 2, 2018

calebcordry Feb 2, 2018

newmuis Feb 2, 2018

calebcordry Feb 2, 2018

newmuis Feb 2, 2018

calebcordry Feb 2, 2018

newmuis Feb 2, 2018

lannka Feb 2, 2018

newmuis Feb 2, 2018

cramforce commented Feb 2, 2018

Story-Auto-Ads Skeleton #13239

Story-Auto-Ads Skeleton #13239

Conversation

calebcordry commented Feb 2, 2018

calebcordry commented Feb 2, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka Feb 2, 2018 • edited

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

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

cramforce commented Feb 2, 2018

lannka Feb 2, 2018 •

edited