Change Story Ad Insertion #18959
Change Story Ad Insertion #18959
Conversation
| } | ||
|
|
||
| // Set to -1 because there is still one page to be viewed before ad. | ||
| this.uniquePagesCount_ = -1; |
There was a problem hiding this comment.
This felt a bit strange, I could be convinced to move this -1 to L601.
There was a problem hiding this comment.
indeed, still don't quite understand why it was initialized as 1 and reset as -1
There was a problem hiding this comment.
We can only count at navigation (click to next page) time. So on the first page we have seen one page even though 0 pages have been navigated to.
We then reset the counter when an ad is inserted. It is inserted one page away, so we see one more page before the ad. Then we still want to wait 8 pages after.
Do you think it would be better to set it always to zero, and have the offset at the check time?
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * start the process over | ||
| * @private | ||
| */ | ||
| startNextPage_() { |
| /** @const */ | ||
| const MIN_INTERVAL = 4; | ||
| /** @const {number} */ | ||
| const FIRST_AD_MIN = 7; |
There was a problem hiding this comment.
does this mean the 8th page can be an ad?
There was a problem hiding this comment.
Yes. 7 pages then ad.
| const FIRST_AD_MIN = 7; | ||
|
|
||
| /** @const {number} */ | ||
| const MIN_INTERVAL = 8; |
There was a problem hiding this comment.
does this mean the 9th page be an ad? or 8th?
There was a problem hiding this comment.
Ninth page should be ad (really its after the 15th then after 23 etc.)
| this.uniquePagesCount_ = 0; | ||
| /** | ||
| * Set to one here because we count on navigation event, and we do have | ||
| * an event before first page. |
There was a problem hiding this comment.
i don't fully understand this. could you rephrase?
There was a problem hiding this comment.
I tried to explain above. Let me know if you think there is a better way to rephrase.
There was a problem hiding this comment.
Rewrote this. LMK if you think it should still be improved.
| * @return {boolean} | ||
| * @private | ||
| */ | ||
| enoughPagesShown_() { |
There was a problem hiding this comment.
enoughContentPagesViewed_()
| @@ -182,6 +189,9 @@ export class AmpStoryAutoAds extends AMP.BaseElement { | |||
| /** @private {number|null} */ | |||
| this.idOfAdShowing_ = null; | |||
|
|
|||
| /** @private {boolean} */ | |||
| this.firstAdShown_ = false; | |||
There was a problem hiding this comment.
let's be consistent about terminology here, i think this mean viewed?
| } | ||
|
|
||
| // Set to -1 because there is still one page to be viewed before ad. | ||
| this.uniquePagesCount_ = -1; |
There was a problem hiding this comment.
indeed, still don't quite understand why it was initialized as 1 and reset as -1
|
A couple extra things after initial review: Moved the logic so that we start next ad loading after view. This eliminates the -1 confusion. Set pages before first ad, and interval thereafter to be the same. I think we should keep the extra logic in case we want to special case the first ad in the future. |
| * @return {boolean} | ||
| * @private | ||
| */ | ||
| enoughContentPagesShown_() { |
* tweak insertion * renaming * start next after view * change interval * closure * test * rename
FR was to have the first ad show after 7 pages, and then after every 8 thereafter.
Tweaked the page counting a bit so that the defined
consts more accurately reflect the page of insertion.