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

✨ <amp-next-page> v2 default and templated separator elements #26413

Merged
merged 29 commits into from Jan 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2c60f47
Added visual diff tests and better manual tests
wassgha Dec 19, 2019
fd8de0d
Prototyping sticky element handling in amp-next-page
wassgha Dec 19, 2019
5733c3e
Merge branch 'master' into amp-next-page-v2-hide-elements
wassgha Dec 19, 2019
35c9f61
Merge branch 'master' into amp-next-page-v2-visual-diff
wassgha Dec 19, 2019
bac73c6
Deprecate amp-next-page-keep in favor of amp-next-page-hidden
wassgha Jan 2, 2020
8295a33
Visibility bug fix
wassgha Jan 2, 2020
bf9e8a6
Remove animation added for testing
wassgha Jan 6, 2020
2085a36
Exported host-page-specific parameters into HostPage
wassgha Jan 8, 2020
aca949b
Merge branch 'master' into amp-next-page-v2-hide-elements
wassgha Jan 8, 2020
72ce30f
Preventing next-page form building again
wassgha Jan 10, 2020
3c38550
Merge branch 'master' into amp-next-page-v2-visual-diff
wassgha Jan 10, 2020
d12c8c6
Remove visual diff tests
wassgha Jan 10, 2020
0a35078
Merge branch 'amp-next-page-v2-visual-diff' into amp-next-page-v2-inf…
wassgha Jan 10, 2020
81a3e93
Implement infinite loading for amp-next-page v2
wassgha Jan 10, 2020
63b4407
Merge branch 'master' into amp-next-page-v2-infinite-loading
wassgha Jan 15, 2020
8065574
Implements un-loading and re-loading pages to reduce memory footprint
wassgha Jan 17, 2020
3959e56
Added unit tests
wassgha Jan 17, 2020
6fb9245
Fixes types
wassgha Jan 20, 2020
6a55cff
Implemented a default separator pill and templating for the separator
wassgha Jan 20, 2020
5bf5fd6
Types fix
wassgha Jan 21, 2020
d3ab4cb
Merge branch 'master' into amp-next-page-v2separators
wassgha Jan 22, 2020
2f19d18
Fix tests for renaming
wassgha Jan 23, 2020
542cc2d
Merge branch 'master' into amp-next-page-v2separators
wassgha Jan 23, 2020
2c1a9aa
Added unit tests for separators
wassgha Jan 23, 2020
909bb9e
Fix types
wassgha Jan 24, 2020
4c182f8
Revert renaming and fixes (separate PR)
wassgha Jan 24, 2020
e773ffc
Merge branch 'master' into amp-next-page-v2separators
wassgha Jan 28, 2020
7178d14
Suggested changes (1/2)
wassgha Jan 29, 2020
a9d0416
Suggested changes (2/2)
wassgha Jan 29, 2020
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
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_());
wassgha marked this conversation as resolved.
Show resolved Hide resolved
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