From 6a55cff6a6ec45f2299741251ff8cc4d0f4942a0 Mon Sep 17 00:00:00 2001 From: Wassim Gharbi Date: Mon, 20 Jan 2020 10:56:40 -0800 Subject: [PATCH] Implemented a default separator pill and templating for the separator --- .../amp-next-page/1.0/amp-next-page.css | 31 ++++++- extensions/amp-next-page/1.0/page.js | 2 +- extensions/amp-next-page/1.0/service.js | 83 +++++++++++++------ .../amp-next-page-v1.element-visibility.html | 10 +-- .../amp-next-page/1.0/amp-next-page.amp.html | 30 ++++++- .../1.0/amp-next-page.element-visibility.html | 10 +-- .../1.0/articles/element-visibility-1.html | 6 +- .../1.0/articles/element-visibility-2.html | 6 +- 8 files changed, 129 insertions(+), 49 deletions(-) diff --git a/extensions/amp-next-page/1.0/amp-next-page.css b/extensions/amp-next-page/1.0/amp-next-page.css index 5368d0f8755d..70fccd11df3c 100644 --- a/extensions/amp-next-page/1.0/amp-next-page.css +++ b/extensions/amp-next-page/1.0/amp-next-page.css @@ -18,16 +18,41 @@ overflow: hidden; } -.i-amphtml-next-page-document:not(.amp-next-page-document-visible) - [i-amphtml-fixedid] { +.i-amphtml-next-page-document:not(.next-page-visible) [i-amphtml-fixedid] { display: none; } -.i-amphtml-next-page-document.amp-next-page-document-visible { +.i-amphtml-next-page-document.next-page-visible { opacity: 1; overflow: visible; } +/** Fixes collapsing margins when switching between the visible and hidden states */ +.i-amphtml-next-page-document:before, +.i-amphtml-next-page-document:after { + content: ' '; + display: table; +} + .i-amphtml-next-page-placeholder { background: #eee; } + +.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; +} diff --git a/extensions/amp-next-page/1.0/page.js b/extensions/amp-next-page/1.0/page.js index 61d6fa36f3bd..67db6cd705b1 100644 --- a/extensions/amp-next-page/1.0/page.js +++ b/extensions/amp-next-page/1.0/page.js @@ -30,7 +30,7 @@ export const PageState = { PAUSED: 5, }; -const VISIBLE_DOC_CLASS = 'amp-next-page-document-visible'; +const VISIBLE_DOC_CLASS = 'next-page-visible'; export class Page { /** diff --git a/extensions/amp-next-page/1.0/service.js b/extensions/amp-next-page/1.0/service.js index 15aad26e701d..931966bd55b4 100644 --- a/extensions/amp-next-page/1.0/service.js +++ b/extensions/amp-next-page/1.0/service.js @@ -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, @@ -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; @@ -383,13 +388,16 @@ export class NextPageService { const container = this.win_.document.createElement('div'); container.classList.add(DOC_CONTAINER_CLASS); + // Insert the separator + const separatorInstance = this.separator_.cloneNode(true); + container.appendChild(separatorInstance); + 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_); @@ -416,7 +424,7 @@ export class NextPageService { */ attachDocumentToPage(page, content, force = false) { // If the user already scrolled to the bottom, prevent rendering - if (this.getViewportsAway_() <= NEAR_BOTTOM_VIEWPORT_COUNT && !force) { + if (this.getViewportsAway_() < NEAR_BOTTOM_VIEWPORT_COUNT && !force) { // TODO(wassgha): Append a "load next article" button? return null; } @@ -533,15 +541,15 @@ export class NextPageService { } /** - * Hides or shows elements based on the `amp-next-page-hide` and - * `amp-next-page-replace` attributes + * Hides or shows elements based on the `next-page-hide` and + * `next-page-replace` attributes * @param {!Document|!ShadowRoot} doc Document of interest * @param {boolean=} isVisible Whether this page is visible or not */ toggleHiddenAndReplaceableElements(doc, isVisible = true) { - // Hide elements that have [amp-next-page-hide] on child documents + // Hide elements that have [next-page-hide] on child documents if (doc !== this.hostPage_.document) { - toArray(doc.querySelectorAll('[amp-next-page-hide]')).forEach(element => + toArray(doc.querySelectorAll('[next-page-hide]')).forEach(element => toggle(element, false /** opt_display */) ); } @@ -551,14 +559,14 @@ export class NextPageService { return; } - // Replace elements that have [amp-next-page-replace] + // Replace elements that have [next-page-replace] toArray( - doc.querySelectorAll('*:not(amp-next-page) [amp-next-page-replace]') + doc.querySelectorAll('*:not(amp-next-page) [next-page-replace]') ).forEach(element => { - let uniqueId = element.getAttribute('amp-next-page-replace'); + let uniqueId = element.getAttribute('next-page-replace'); if (!uniqueId) { uniqueId = String(Date.now() + Math.floor(Math.random() * 100)); - element.setAttribute('amp-next-page-replace', uniqueId); + element.setAttribute('next-page-replace', uniqueId); } if ( @@ -725,11 +733,7 @@ export class NextPageService { * @private */ getSeparatorElement_(element) { - const providedSeparator = childElementByAttr( - element, - 'amp-next-page-separator' - ); - // TODO(wassgha): Use templates (amp-mustache) to render the separator + const providedSeparator = childElementByAttr(element, 'separator'); if (providedSeparator) { removeElement(providedSeparator); } @@ -741,9 +745,41 @@ export class NextPageService { * @private */ buildDefaultSeparator_() { - const separator = this.win_.document.createElement('div'); - separator.classList.add('amp-next-page-separator'); - return separator; + const html = htmlFor(this.element_); + return html` +
+ Next article +
+ `; + } + + /** + * 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(); + } + this.templates_ + .findAndRenderTemplate(separator, { + title: page.title, + url: page.url, + image: page.image, + }) + .then(rendered => { + return this.mutator_.mutateElement(separator, () => { + removeChildren(dev().assertElement(separator)); + separator.appendChild(rendered); + }); + }); } /** @@ -752,10 +788,7 @@ export class NextPageService { * @private */ getMoreBoxElement_(element) { - const providedMoreBox = childElementByAttr( - element, - 'amp-next-page-more-box' - ); + const providedMoreBox = childElementByAttr(element, 'more-box'); // TODO(wassgha): Use templates (amp-mustache) to render the more box if (providedMoreBox) { removeElement(providedMoreBox); @@ -770,7 +803,7 @@ export class NextPageService { buildDefaultMoreBox_() { // TODO(wassgha): Better default more box const moreBox = this.win_.document.createElement('div'); - moreBox.classList.add('amp-next-page-more-box'); + moreBox.classList.add('amp-next-page-default-more-box'); return moreBox; } } diff --git a/test/manual/amp-next-page/1.0/amp-next-page-v1.element-visibility.html b/test/manual/amp-next-page/1.0/amp-next-page-v1.element-visibility.html index 61043240fa63..46da3553f2b4 100644 --- a/test/manual/amp-next-page/1.0/amp-next-page-v1.element-visibility.html +++ b/test/manual/amp-next-page/1.0/amp-next-page-v1.element-visibility.html @@ -62,7 +62,7 @@

Content discovery

varius est suscipit vitae. Maecenas ut sapien diam. Vivamus viverra nisl at quam pellentesque posuere. Cras ut nibh non arcu dignissim elementum.

- +

Vestibulum a nisi est. Fusce consequat iaculis vehicula. Aliquam luctus nunc risus, ut congue augue tristique nec. In venenatis aliquam tristique. @@ -77,8 +77,8 @@

Content discovery

enim, non pellentesque sem. Nam non sem lacinia, lacinia nisi non, consectetur enim.

-
I should get replaced by my sibling from each loaded page (page 0)
-
I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 0)
+
I should get replaced by my sibling from each loaded page (page 0)
+
I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 0)

Maecenas sed quam nec dolor rhoncus rutrum ut non mauris. Pellentesque ante dolor, pretium quis enim eu, condimentum volutpat augue. Donec nulla @@ -306,10 +306,10 @@

Content discovery

] } -
+
Read more
-
+
Here are a few more articles ...
diff --git a/test/manual/amp-next-page/1.0/amp-next-page.amp.html b/test/manual/amp-next-page/1.0/amp-next-page.amp.html index 586fd4d6d365..556b39ee868f 100644 --- a/test/manual/amp-next-page/1.0/amp-next-page.amp.html +++ b/test/manual/amp-next-page/1.0/amp-next-page.amp.html @@ -9,6 +9,7 @@ + -
- Read more +
+
-
+
Here are a few more articles ...
diff --git a/test/manual/amp-next-page/1.0/amp-next-page.element-visibility.html b/test/manual/amp-next-page/1.0/amp-next-page.element-visibility.html index f28ad3ff02a1..88367f4ec1ec 100644 --- a/test/manual/amp-next-page/1.0/amp-next-page.element-visibility.html +++ b/test/manual/amp-next-page/1.0/amp-next-page.element-visibility.html @@ -61,7 +61,7 @@

Content discovery

varius est suscipit vitae. Maecenas ut sapien diam. Vivamus viverra nisl at quam pellentesque posuere. Cras ut nibh non arcu dignissim elementum.

- +

Vestibulum a nisi est. Fusce consequat iaculis vehicula. Aliquam luctus nunc risus, ut congue augue tristique nec. In venenatis aliquam tristique. @@ -76,8 +76,8 @@

Content discovery

enim, non pellentesque sem. Nam non sem lacinia, lacinia nisi non, consectetur enim.

-
I should get replaced by my sibling from each loaded page (page 0)
-
I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 0)
+
I should get replaced by my sibling from each loaded page (page 0)
+
I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 0)

Maecenas sed quam nec dolor rhoncus rutrum ut non mauris. Pellentesque ante dolor, pretium quis enim eu, condimentum volutpat augue. Donec nulla @@ -300,10 +300,10 @@

Content discovery

} ] -
+
Read more
-
+
Here are a few more articles ...
diff --git a/test/manual/amp-next-page/1.0/articles/element-visibility-1.html b/test/manual/amp-next-page/1.0/articles/element-visibility-1.html index 05fde086375b..e1bc7ea29a7e 100644 --- a/test/manual/amp-next-page/1.0/articles/element-visibility-1.html +++ b/test/manual/amp-next-page/1.0/articles/element-visibility-1.html @@ -55,7 +55,7 @@

Content discovery

varius est suscipit vitae. Maecenas ut sapien diam. Vivamus viverra nisl at quam pellentesque posuere. Cras ut nibh non arcu dignissim elementum.

- +

Vestibulum a nisi est. Fusce consequat iaculis vehicula. Aliquam luctus nunc risus, ut congue augue tristique nec. In venenatis aliquam tristique. @@ -70,8 +70,8 @@

Content discovery

enim, non pellentesque sem. Nam non sem lacinia, lacinia nisi non, consectetur enim.

-
I got replaced by my sibling from page 1
-
I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 1)
+
I got replaced by my sibling from page 1
+
I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 1)

Maecenas sed quam nec dolor rhoncus rutrum ut non mauris. Pellentesque ante dolor, pretium quis enim eu, condimentum volutpat augue. Donec nulla diff --git a/test/manual/amp-next-page/1.0/articles/element-visibility-2.html b/test/manual/amp-next-page/1.0/articles/element-visibility-2.html index 5dd22b3e3801..e9180fa9b83d 100644 --- a/test/manual/amp-next-page/1.0/articles/element-visibility-2.html +++ b/test/manual/amp-next-page/1.0/articles/element-visibility-2.html @@ -55,7 +55,7 @@

Content discovery

varius est suscipit vitae. Maecenas ut sapien diam. Vivamus viverra nisl at quam pellentesque posuere. Cras ut nibh non arcu dignissim elementum.

- +

Vestibulum a nisi est. Fusce consequat iaculis vehicula. Aliquam luctus nunc risus, ut congue augue tristique nec. In venenatis aliquam tristique. @@ -70,8 +70,8 @@

Content discovery

enim, non pellentesque sem. Nam non sem lacinia, lacinia nisi non, consectetur enim.

-
I got replaced by my sibling from page 2
-
I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 2)
+
I got replaced by my sibling from page 2
+
I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 2)

Maecenas sed quam nec dolor rhoncus rutrum ut non mauris. Pellentesque ante dolor, pretium quis enim eu, condimentum volutpat augue. Donec nulla