Implement new loader for ads #23687
Implement new loader for ads #23687
Conversation
- Move some presentation logic from JS into CSS - Change custom logos to define their own SVGs * This separates concerns better, allowing the logos to specify their own viewbox for example - Make API for custom loaders specifying a color more explicit (rather than just reading it off the custom loader Element) - Change coordinate systems for the logos to start at zero so they are easier to reason about. - Fix the default loader size, which defaults to 300x300 on Firefox when no size is specified. - Fix the smaller loader being the wrong size, due to the SVG being sized down. This may overflow the parent container, but should position correctly as it is centered.
- Ad localDev flag to test page.
| // Not Implemented | ||
| return false; | ||
| return ( | ||
| this.element_.tagName == 'AMP-AD' || this.element_.tagName == 'AMP-EMBED' |
There was a problem hiding this comment.
Current behavior: we don't apply placeholder to <amp-embed>, because <amp-embed> component is usually pretty long, multiple viewport height. And we figure it's not good UI to apply loader in such case.
https://github.com/ampproject/amphtml/blob/master/extensions/amp-ad/0.1/amp-ad-ui.js#L143
But I know @lannka propose that we remove all differences between <amp-ad> and <amp-embed> and make them exactly the same component.
Can we please keep the same behavior for now (not adding loader to <amp-embed>. I'm open to future change if we agree on the loader UI for elements that's longer than 100vh. Thanks.
There was a problem hiding this comment.
After taking a look, I noticed that <amp-embed> currently gets the 3 dot loader (and not the ads loader). The loader element is due to the inclusion in this list:
Lines 94 to 109 in ff8827b
which was added in 919ba58
Since we want to remove the legacy loader, I think we should either:
- Move forward with the new ads loader for
<amp-embed> - Move forward with the new generic loader for
<amp-embed> - Remove the the loader element from
<amp-embed>as part of the experiment / ramp up to measure the impact.
Any thoughts on which would be preferred?
There was a problem hiding this comment.
Per our discussion, we will go with the generic loader for now, since that matches the current intent / implementation.
| */ | ||
| createPlaceholder() { | ||
| if (isNewLoaderExperimentEnabled(this.element_)) { |
There was a problem hiding this comment.
What's the current plan on the new loader experiment? Curious what data we collect from the experiment.
cc @keithwrightbos Do you see any potential impact on ads side with the default loader change? Thanks.
There was a problem hiding this comment.
The current plan is to have a ramp up schedule of 1%, 10%, 50%, and then 100% with a minimum one week spacing at each stage to verify metrics are not impacted beyond what we might reasonably expect and to understand the scope of the impact.
@nainar Who might have more context.
There was a problem hiding this comment.
What's our decision on the loader and placeholder. If we decide to keep the ad icon as the placeholder, should we keep the old logic here because we will always need the ad icon
There was a problem hiding this comment.
The new loader renders the "ad" icon as a part of the loader (in the middle of the spinner). So in that case, we don't want to create the placeholder here. Eventually, we should remove the default placeholder logic.
There was a problem hiding this comment.
I see. Thanks
In this case, can I assume the ad icon will still be displayed on top of the publisher provided placeholder.
There was a problem hiding this comment.
Yep, we'll display a circle with a dark (but translucent) grey background on top of the placeholder, then the spinner / logo on top of that.
- This makes sure that the logo, text are truely centered and gives more leeway for font rendering.
- Implement new loader for ads (amp-ad) - Make amp-embed use the default loader
localDev, which is needed when starting a local server.