Load sticky ad after first viewport scroll#7478
Conversation
| const getSizeSpy = sandbox.spy(); | ||
| const getScrollHeightSpy = sandbox.spy(); | ||
|
|
||
| impl.viewport_.getScrollTop = function() { |
There was a problem hiding this comment.
const getScrollTopStub = sandbox.stub(impl.viewport_, 'getScrollTop');
getScrollTopStub.returns(1);
|
Closes #6597? Please update the description to say so. 😀 |
| } from '../../../src/style'; | ||
|
|
||
| /** @const */ | ||
| const TAG = 'sticky-ad-early-load'; |
There was a problem hiding this comment.
TAG is not the right metaphor here, maybe EARLY_LOAD_EXPERIMENT ?
| * Display and load sticky ad. | ||
| * @private | ||
| */ | ||
| displayAfterScroll_() { |
| displayAfterScroll_() { | ||
| onScroll_() { | ||
| if (isExperimentOn(this.win, TAG)) { | ||
| this.displayAfterScroll_(); |
There was a problem hiding this comment.
This would show it as soon as scrolling starts, we want to show the ad when scrolling ends, resources-impl already has some logic for detecting scroll stop, it would be nice to refactor that into a common helper to use here as well. I think a var scrollEndUnlisten = this.viewport_.onScrollEnd() would be a nice API to have that can be used by this and resources-impl
There was a problem hiding this comment.
As agreed #6597 (comment) want to show the ad after first user interaction. As long as scroll slows down and resources manager can handle this mutate, it is OK to display before scroll fully stops.
#deferMutate already does the work for me.
There was a problem hiding this comment.
Whoa! didn't know #deferMutate does that. AMAZING.
There was a problem hiding this comment.
#deferMutate will only helps to mutate callback to execute at earliest point. But resources manager won't measure elements until scroll velocity decrease to certain threshold. so i think it's fine to use this.viewport_.onScroll() as resources manager already handle the optimization for me.
I am also OK with using this.viewport_.onChanged() as on changed will only be fired after scroll slows down.
https://github.com/ampproject/amphtml/blob/master/src/service/viewport-impl.js#L713
|
👀 |
* add experiment flag * add cleanup issue * address comment * fix lint * nit
Closes #6597
This PR introduces an experiment flag 'sticky-ad-early-load'
Now: load sticky ad after viewport scrolls down 1vp length
Experiment: load sticky ad after first viewport scroll