Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚮Cleanup for the new loaders. #24571

Merged
merged 2 commits into from
Sep 24, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion build-system/global-configs/canary-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
"ios-fixed-no-transfer": 1,
"ios-scrollable-iframe": 0,
"macro-after-long-task": 1,
"new-loaders": 1,
"pump-early-frame": 1,
"version-locking": 1
}
1 change: 0 additions & 1 deletion build-system/global-configs/prod-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
"ios-fixed-no-transfer": 0,
"ios-scrollable-iframe": 0,
"macro-after-long-task": 0,
"new-loaders": 1,
"pump-early-frame": 1,
"version-locking": 1
}
90 changes: 0 additions & 90 deletions css/ampshared.css
Original file line number Diff line number Diff line change
Expand Up @@ -334,100 +334,10 @@ i-amphtml-scroll-container.amp-active, i-amp-scroll-container.amp-active {
display: block !important;
}

/**
* `i-amphtml-loading-container`, `i-amphtml-loader` and `i-amphtml-loader-dot` all support
* a "loading indicator" usecase. `i-amphtml-loading-container` is mostly responsible
* for aligning the loading indicator, while `i-amphtml-loader` and
* `i-amphtml-loader-dot` are an implementation for a default loading indicator. The
* default implementation includes the three-dot layout and animation.
*/
.i-amphtml-loading-container.amp-hidden {
visibility: hidden;
}

.i-amphtml-loader-line {
position: absolute;
top:0;
left: 0;
right: 0;
height: 1px;
overflow: hidden !important;
background-color:rgba(151, 151, 151, 0.2);
display: block;
}

.i-amphtml-loader-moving-line {
display: block;
position: absolute;
width: 100%;
height: 100% !important;
background-color: rgba(151, 151, 151, 0.65);
z-index: 2;
}

@keyframes i-amphtml-loader-line-moving {
0% {transform: translateX(-100%);}
100% {transform: translateX(100%);}
}

.i-amphtml-loader-line.amp-active .i-amphtml-loader-moving-line {
animation: i-amphtml-loader-line-moving 4s ease infinite;
}

.i-amphtml-loader {
position: absolute;
display: block;
height: 10px;
top: 50%;
left: 50%;
transform: translateX(-50%) translateY(-50%);
transform-origin: 50% 50%;
white-space: nowrap;
}

.i-amphtml-loader.amp-active .i-amphtml-loader-dot {
animation: i-amphtml-loader-dots 2s infinite;
}

.i-amphtml-loader-dot {
position: relative;
display: inline-block;

height: 10px;
width: 10px;
margin: 2px;
border-radius: 100%;
background-color: rgba(0, 0, 0, .3);

box-shadow: 2px 2px 2px 1px rgba(0, 0, 0, .2);
will-change: transform;
}

.i-amphtml-loader .i-amphtml-loader-dot:nth-child(1) {
animation-delay: 0s;
}

.i-amphtml-loader .i-amphtml-loader-dot:nth-child(2) {
animation-delay: .1s;
}

.i-amphtml-loader .i-amphtml-loader-dot:nth-child(3) {
animation-delay: .2s;
}

@keyframes i-amphtml-loader-dots {
0%, 100% {
transform: scale(.7);
background-color: rgba(0, 0, 0, .3);
}

50% {
transform: scale(.8);
background-color: rgba(0, 0, 0, .5);
}
}


/**
* "overflow" element is an element shown when more content is available but
* not currently visible. Typically tapping on this element shows the full
Expand Down
5 changes: 0 additions & 5 deletions extensions/amp-ad/0.1/amp-ad-3p-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -441,11 +441,6 @@ export class AmpAd3PImpl extends AMP.BaseElement {
return true;
}

/** @override */
createPlaceholderCallback() {
return this.uiHandler.createPlaceholder();
}

/**
* @return {!Promise<?CONSENT_POLICY_STATE>}
*/
Expand Down
5 changes: 0 additions & 5 deletions extensions/amp-ad/0.1/amp-ad-custom.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,6 @@ export class AmpAdCustom extends AMP.BaseElement {
return true;
}

/** @override */
createPlaceholderCallback() {
return this.uiHandler.createPlaceholder();
}

/**
* @private getFullUrl_ Get a URL which includes a parameter indicating
* all slots to be fetched from this web server URL
Expand Down
14 changes: 0 additions & 14 deletions extensions/amp-ad/0.1/amp-ad-ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

import {ancestorElementsByTag} from '../../../src/dom';
import {getAdContainer} from '../../../src/ad-helper';
import {isNewLoaderExperimentEnabled} from '../../../src/loader';
import {isProxyOrigin} from '../../../src/url';
import {user} from '../../../src/log';

Expand Down Expand Up @@ -60,19 +59,6 @@ export class AmpAdUIHandler {
}
}

/**
* Create a default placeholder if not provided.
* Should be called in baseElement createPlaceholderCallback.
* @return {?Element}
*/
createPlaceholder() {
sparhami marked this conversation as resolved.
Show resolved Hide resolved
if (isNewLoaderExperimentEnabled(this.element_)) {
return null;
}

return this.addDefaultUiComponent_('placeholder');
}

/**
* Apply UI for laid out ad with no-content
* Order: try collapse -> apply provided fallback -> apply default fallback
Expand Down
51 changes: 7 additions & 44 deletions src/custom-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,7 @@ import {ResourceState} from './service/resource';
import {Services} from './services';
import {Signals} from './utils/signals';
import {blockedByConsentError, isBlockedByConsent, reportError} from './error';
import {
createLegacyLoaderElement,
createNewLoaderElement,
isNewLoaderExperimentEnabled,
} from '../src/loader.js';
import {createLoaderElement} from '../src/loader.js';
import {dev, devAssert, rethrowAsync, user, userAssert} from './log';
import {getIntersectionChangeEntry} from '../src/intersection-observer-polyfill';
import {getMode} from './mode';
Expand All @@ -48,15 +44,6 @@ import {tryResolve} from '../src/utils/promise';

const TAG = 'CustomElement';

/**
* This is the minimum width of the element needed to trigger `loading`
* animation. This value is justified as about 1/3 of a smallish mobile
* device viewport. Trying to put a loading indicator into a small element
* is meaningless.
* @private @const {number}
*/
const MIN_WIDTH_FOR_LOADING = 100;

/**
* The elements positioned ahead of this threshold may have their loading
* indicator initialized faster. This is benefitial to avoid relayout during
Expand Down Expand Up @@ -1663,7 +1650,6 @@ function createBaseCustomElementClass(win) {
this.layoutWidth_ <= 0 || // Layout is not ready or invisible
this.loadingDisabled_ ||
!isLoadingAllowed(this) ||
isTooSmallForLoader(this) ||
isInternalOrServiceNode(this) ||
!isLayoutSizeDefined(this.layout_)
) {
Expand Down Expand Up @@ -1702,21 +1688,12 @@ function createBaseCustomElementClass(win) {
const container = htmlFor(/** @type {!Document} */ (doc))`
<div class="i-amphtml-loading-container i-amphtml-fill-content
amp-hidden"></div>`;

let loadingElement;
if (isNewLoaderExperimentEnabled(this)) {
loadingElement = createNewLoaderElement(
devAssert(this.ampdoc_),
this,
this.layoutWidth_,
this.layoutHeight_
);
} else {
loadingElement = createLegacyLoaderElement(
/** @type {!Document} */ (doc),
this.elementName()
);
}
const loadingElement = createLoaderElement(
this.getAmpDoc(),
this,
this.layoutWidth_,
this.layoutHeight_
);

container.appendChild(loadingElement);

Expand Down Expand Up @@ -1919,20 +1896,6 @@ function isInternalOrServiceNode(node) {
return false;
}

/**
* Whether element size is too small to show loader.
* @param {!Element} element
* @return {boolean}
*/
function isTooSmallForLoader(element) {
// New loaders experiments has its own sizing heuristics
if (isNewLoaderExperimentEnabled(element)) {
return false;
}

return element.layoutWidth_ < MIN_WIDTH_FOR_LOADING;
}

/**
* Creates a new custom element class prototype.
*
Expand Down
49 changes: 3 additions & 46 deletions src/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,39 +15,7 @@
*/

import {Services} from './services';
import {devAssert} from './log';
import {htmlFor} from './static-template';
import {isExperimentOn} from './experiments';
import {toWin} from './types';

/* LEGACY LOADER */

/** @private @const */
const LINE_LOADER_ELEMENTS = {
'AMP-AD': true,
};

/**
* Creates a default "loading indicator" element. This element accepts
* `amp-active` class in which case it may choose to run an animation.
* @param {!Document} doc
* @param {string} elementName
* @return {!Element}
*/
export function createLegacyLoaderElement(doc, elementName) {
if (LINE_LOADER_ELEMENTS[elementName.toUpperCase()]) {
return htmlFor(doc)`<div class="i-amphtml-loader-line">
<div class="i-amphtml-loader-moving-line"></div>
</div>`;
}
return htmlFor(doc)`<div class="i-amphtml-loader">
<div class="i-amphtml-loader-dot"></div>
<div class="i-amphtml-loader-dot"></div>
<div class="i-amphtml-loader-dot"></div>
</div>`;
}

/* NEW LOADER */
/** @type {?Promise<!../extensions/amp-loader/0.1/amp-loader.LoaderService>} */
let loaderServicePromise = null;

Expand Down Expand Up @@ -77,18 +45,17 @@ function getLoaderServicePromise(ampDoc, element) {
* @param {!AmpElement} element
* @param {number} elementWidth
* @param {number} elementHeight
* @return {!Element} New loader root element
* @return {!Element} The loader root element.
*/
export function createNewLoaderElement(
export function createLoaderElement(
ampDoc,
element,
elementWidth,
elementHeight
) {
devAssert(isNewLoaderExperimentEnabled(element));
const startTime = Date.now();
// We create the loader root element up front, since it is needed
// synchronously. We create the actually element with animations when the
// synchronously. We create the actual element with animations when the
// service is ready.
const loaderRoot = element.ownerDocument.createElement('div');

Expand All @@ -107,13 +74,3 @@ export function createNewLoaderElement(

return loaderRoot;
}

/**
* Whether the new loader experiment is enabled.
* @param {!AmpElement} element
* @return {boolean}
*/
export function isNewLoaderExperimentEnabled(element) {
const win = toWin(element.ownerDocument.defaultView);
return isExperimentOn(win, 'new-loaders');
}
11 changes: 0 additions & 11 deletions test/manual/loaders-new.amp.html → test/manual/loaders.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,6 @@
},
true
);

function toggleExp(name) {
const isOn = AMP.isExperimentOn(name);
AMP.toggleExperiment(name, !isOn);
window.location.reload();
}
</script>
<script src="https://cdn.ampproject.org/v0.js"></script>
<script
Expand Down Expand Up @@ -327,11 +321,6 @@
- You can change the delay by adding ?{number} (e.g. ?9000) to the Url
</div>
</div>
<div class="toolbar">
<button onclick="toggleExp('new-loaders')">
Toggle New/Old Default Loaders
</button>
</div>

<amp-selector class="tabs-with-flex" role="tablist">
<div role="tab" option selected>
Expand Down