Skip to content

Commit

Permalink
✨ <amp-next-page> v2 default and templated separator elements (#26413)
Browse files Browse the repository at this point in the history
* Added visual diff tests and better  manual tests

* Prototyping sticky element handling in amp-next-page

* Deprecate amp-next-page-keep in favor of amp-next-page-hidden

* Visibility bug fix

* Remove animation added for testing

* Exported host-page-specific parameters into HostPage

* Preventing next-page form building again

* Remove visual diff tests

* Implement infinite loading for amp-next-page v2

* Implements un-loading and re-loading pages to reduce memory footprint

* Added unit tests

* Fixes types

* Implemented a default separator pill and templating for the separator

* Types fix

* Fix tests for renaming

* Added unit tests for separators

* Fix types

* Revert renaming and fixes (separate PR)

* Suggested changes (1/2)

* Suggested changes (2/2)
  • Loading branch information
wassgha committed Jan 31, 2020
1 parent be3317a commit e7a8e7f
Show file tree
Hide file tree
Showing 6 changed files with 657 additions and 121 deletions.
19 changes: 19 additions & 0 deletions extensions/amp-next-page/1.0/amp-next-page.css
Expand Up @@ -27,6 +27,25 @@
overflow: visible;
}

.amp-next-page-default-separator {
position: relative;
display: flex;
flex-direction: row;
justify-content: center;
align-items: center;
text-align: center;
font-family: 'Roboto', Helvetica, Arial, sans-serif;
font-weight: 400;
font-size: 14px;
width: 160px;
height: 38px;
margin: auto;
border-radius: 32px;
box-shadow: 0px 6px 17px 0px rgba(0, 0, 0, 0.09);
background: black;
color: white;
}

/** Fixes collapsing margins when switching between the visible and hidden states */
.i-amphtml-next-page-document:before,
.i-amphtml-next-page-document:after {
Expand Down
20 changes: 10 additions & 10 deletions extensions/amp-next-page/1.0/page.js
Expand Up @@ -53,7 +53,7 @@ export class Page {
/** @private {?Element} */
this.container_ = null;
/** @private {?Document} */
this.cachedContent_ = null;
this.content_ = null;
/** @private {!PageState} */
this.state_ = PageState.QUEUED;
/** @private {!VisibilityState} */
Expand Down Expand Up @@ -193,7 +193,7 @@ export class Page {
* root has been removed
*/
resume() {
this.attach_(devAssert(this.cachedContent_));
this.attach_();
}

/**
Expand Down Expand Up @@ -234,13 +234,14 @@ export class Page {
.fetchPageDocument(this)
.then(content => {
this.state_ = PageState.LOADED;
this.container_ = this.manager_.createDocumentContainerForPage(
this /** page */
);
this.content_ = content;
return this.manager_.createDocumentContainerForPage(this /** page */);
})
.then(container => {
this.container_ = container;
// TODO(wassgha): To further optimize, this should ideally
// be parsed from the service worker instead of stored in memory
this.cachedContent_ = content;
this.attach_(content);
this.attach_();
})
.catch(() => {
this.state_ = PageState.FAILED;
Expand All @@ -251,12 +252,11 @@ export class Page {

/**
* Inserts the fetched (or cached) HTML as the document's content
* @param {!Document} content
*/
attach_(content) {
attach_() {
const shadowDoc = this.manager_.attachDocumentToPage(
this /** page */,
content,
/** @type {!Document} */ (devAssert(this.content_)),
this.isPaused() /** force */
);

Expand Down
80 changes: 65 additions & 15 deletions extensions/amp-next-page/1.0/service.js
Expand Up @@ -23,11 +23,13 @@ import {
childElementByAttr,
childElementsByTag,
isJsonScriptTag,
removeChildren,
removeElement,
scopedQuerySelector,
} from '../../../src/dom';
import {dev, devAssert, user, userAssert} from '../../../src/log';
import {escapeCssSelectorIdent} from '../../../src/css';
import {htmlFor} from '../../../src/static-template';
import {installStylesForDoc} from '../../../src/style-installer';
import {
parseFavicon,
Expand Down Expand Up @@ -77,6 +79,9 @@ export class NextPageService {
*/
this.mutator_ = Services.mutatorForDoc(ampdoc);

/** @private @const {!../../../src/service/template-impl.Templates} */
this.templates_ = Services.templatesFor(this.win_);

/** @private {?Element} */
this.separator_ = null;

Expand Down Expand Up @@ -380,22 +385,26 @@ export class NextPageService {
* Create a container element for the document and insert it into
* the amp-next-page element
* @param {!Page} page
* @return {!Element}
* @return {!Promise<!Element>}
*/
createDocumentContainerForPage(page) {
const container = this.win_.document.createElement('div');
container.classList.add(DOC_CONTAINER_CLASS);
this.element_.insertBefore(container, dev().assertElement(this.moreBox_));

// Insert the separator
const separatorInstance = this.separator_.cloneNode(true);
container.appendChild(separatorInstance);
const separatorRenderPromise = this.maybeRenderSeparatorTemplate_(
separatorInstance,
page
);

// Insert the document
const shadowRoot = this.win_.document.createElement('div');
shadowRoot.classList.add(SHADOW_ROOT_CLASS);
container.appendChild(shadowRoot);

// Insert the separator
container.appendChild(this.separator_.cloneNode(true));

// Insert the container
this.element_.insertBefore(container, this.moreBox_);

// Observe this page's visibility
this.visibilityObserver_.observe(
shadowRoot /** element */,
Expand All @@ -406,7 +415,7 @@ export class NextPageService {
}
);

return container;
return separatorRenderPromise.then(() => container);
}

/**
Expand Down Expand Up @@ -632,7 +641,10 @@ export class NextPageService {
doc.close();
return doc;
})
.catch(e => user().error(TAG, 'failed to fetch %s', page.url, e));
.catch(e => {
user().error(TAG, 'failed to fetch %s', page.url, e);
throw e;
});
}

/**
Expand Down Expand Up @@ -715,11 +727,17 @@ export class NextPageService {
'be inside a <script> tag with type="application/json"'
);

const pages = tryParseJson(scriptElement.textContent, error => {
const parsed = tryParseJson(scriptElement.textContent, error => {
user().error(TAG, 'failed to parse inline page list', error);
});

return user().assertArray(pages, `${TAG} page list should be an array`);
const pages = user().assertArray(
parsed,
`${TAG} page list should be an array`
);

removeElement(scriptElement);
return pages;
}

/**
Expand All @@ -731,7 +749,6 @@ export class NextPageService {
*/
getSeparatorElement_(element) {
const providedSeparator = childElementByAttr(element, 'separator');
// TODO(wassgha): Use templates (amp-mustache) to render the separator
if (providedSeparator) {
removeElement(providedSeparator);
}
Expand All @@ -743,9 +760,42 @@ export class NextPageService {
* @private
*/
buildDefaultSeparator_() {
const separator = this.win_.document.createElement('div');
separator.classList.add('amp-next-page-default-separator');
return separator;
const html = htmlFor(this.getHostNextPageElement_());
return html`
<div
class="amp-next-page-default-separator"
aria-label="Next article separator"
>
Next article
</div>
`;
}

/**
* Renders the template inside the separator element using
* data from the current article (if a template is present)
*
* @param {!Element} separator
* @param {!Page} page
* @return {!Promise}
*/
maybeRenderSeparatorTemplate_(separator, page) {
if (!this.templates_.hasTemplate(separator)) {
return Promise.resolve();
}
const data = /** @type {!JsonObject} */ ({
title: page.title,
url: page.url,
image: page.image,
});
return this.templates_
.findAndRenderTemplate(separator, data)
.then(rendered => {
return this.mutator_.mutateElement(separator, () => {
removeChildren(dev().assertElement(separator));
separator.appendChild(rendered);
});
});
}

/**
Expand Down

0 comments on commit e7a8e7f

Please sign in to comment.