♻️<amp-next-page> v2 minor renaming and fixes (#26468)

* 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 * Preserve only name changes and fixes * Suggested fixes
ampproject · Jan 28, 2020 · f7a7a2d · f7a7a2d
1 parent bba42e1
commit f7a7a2d
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 46 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
@@ -18,16 +18,22 @@
   overflow: hidden;
 }
 
-.i-amphtml-next-page-document:not(.amp-next-page-document-visible)
-  [i-amphtml-fixedid] {
+.i-amphtml-next-page-document:not(.amp-next-page-visible) [i-amphtml-fixedid] {
   display: none;
 }
 
-.i-amphtml-next-page-document.amp-next-page-document-visible {
+.i-amphtml-next-page-document.amp-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;
 }
diff --git a/extensions/amp-next-page/1.0/page.js b/extensions/amp-next-page/1.0/page.js
@@ -28,7 +28,8 @@ export const PageState = {
   PAUSED: 5,
 };
 
-const VISIBLE_DOC_CLASS = 'amp-next-page-document-visible';
+export const VISIBLE_DOC_CLASS = 'amp-next-page-visible';
+export const HIDDEN_DOC_CLASS = 'amp-next-page-hidden';
 
 export class Page {
   /**
@@ -158,6 +159,9 @@ export class Page {
       this.shadowDoc_.ampdoc
         .getBody()
         .classList.toggle(VISIBLE_DOC_CLASS, this.isVisible());
+      this.shadowDoc_.ampdoc
+        .getBody()
+        .classList.toggle(HIDDEN_DOC_CLASS, !this.isVisible());
     }
 
     if (this.isVisible()) {

diff --git a/extensions/amp-next-page/1.0/service.js b/extensions/amp-next-page/1.0/service.js
@@ -15,7 +15,7 @@
  */
 
 import {CSS} from '../../../build/amp-next-page-1.0.css';
-import {HostPage, Page, PageState} from './page';
+import {HIDDEN_DOC_CLASS, HostPage, Page, PageState} from './page';
 import {MultidocManager} from '../../../src/multidoc-manager';
 import {Services} from '../../../src/services';
 import {VisibilityState} from '../../../src/visibility-state';
@@ -419,7 +419,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;
     }
@@ -531,20 +531,23 @@ export class NextPageService {
       removeElement(el);
     });
 
+    // Mark document as hidden initially
+    doc.body.classList.add(HIDDEN_DOC_CLASS);
+
     // Make sure all hidden elements are initially invisible
     this.toggleHiddenAndReplaceableElements(doc, false /** isVisible */);
   }
 
   /**
-   * 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 to attach.
    * @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 */)
       );
     }
@@ -554,14 +557,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 (
@@ -727,10 +730,7 @@ export class NextPageService {
    * @private
    */
   getSeparatorElement_(element) {
-    const providedSeparator = childElementByAttr(
-      element,
-      'amp-next-page-separator'
-    );
+    const providedSeparator = childElementByAttr(element, 'separator');
     // TODO(wassgha): Use templates (amp-mustache) to render the separator
     if (providedSeparator) {
       removeElement(providedSeparator);
@@ -744,7 +744,7 @@ export class NextPageService {
    */
   buildDefaultSeparator_() {
     const separator = this.win_.document.createElement('div');
-    separator.classList.add('amp-next-page-separator');
+    separator.classList.add('amp-next-page-default-separator');
     return separator;
   }
 
@@ -754,10 +754,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);
@@ -772,7 +769,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/extensions/amp-next-page/1.0/test/test-amp-next-page.js b/extensions/amp-next-page/1.0/test/test-amp-next-page.js
@@ -301,7 +301,7 @@ describes.realWin(
 
         env.fetchMock.get(
           /\/document1/,
-          `${MOCK_NEXT_PAGE} <div amp-next-page-hide id="hidden" />`
+          `${MOCK_NEXT_PAGE} <div next-page-hide id="hidden" />`
         );
         await service.maybeFetchNext();
 
@@ -315,14 +315,14 @@ describes.realWin(
 
         env.fetchMock.get(
           /\/document1/,
-          `${MOCK_NEXT_PAGE} <div amp-next-page-replace="replace-me" instance="1" />`
+          `${MOCK_NEXT_PAGE} <div next-page-replace="replace-me" instance="1" />`
         );
         await service.maybeFetchNext();
         service.pages_[1].setVisibility(VisibilityState.VISIBLE);
 
         env.fetchMock.get(
           /\/document2/,
-          `${MOCK_NEXT_PAGE} <div amp-next-page-replace="replace-me" instance="2" />`
+          `${MOCK_NEXT_PAGE} <div next-page-replace="replace-me" instance="2" />`
         );
         await service.maybeFetchNext();
         service.pages_[1].relativePos = ViewportRelativePos.INSIDE_VIEWPORT;

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
@@ -62,7 +62,7 @@ <h2>Content discovery</h2>
         varius est suscipit vitae. Maecenas ut sapien diam. Vivamus viverra nisl
         at quam pellentesque posuere. Cras ut nibh non arcu dignissim elementum.
       </p>
-      <div class="hidden" amp-next-page-hide>I should be visible when loaded as a host page but hidden if loaded as a next-page</div>
+      <div class="hidden" next-page-hide>I should be visible when loaded as a host page but hidden if loaded as a next-page</div>
       <p>
         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 @@ <h2>Content discovery</h2>
         enim, non pellentesque sem. Nam non sem lacinia, lacinia nisi non,
         consectetur enim.
       </p>
-      <div class="sticky" amp-next-page-replace="my-sticky-element">I should get replaced by my sibling from each loaded page (page 0)</div>
-      <div class="fixed" amp-next-page-hide>I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 0)</div>
+      <div class="sticky" next-page-replace="my-sticky-element">I should get replaced by my sibling from each loaded page (page 0)</div>
+      <div class="fixed" next-page-hide>I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 0)</div>
       <p>
         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 @@ <h2>Content discovery</h2>
             ]
         }
       </script>
-      <div amp-next-page-separator>
+      <div separator>
           Read more
       </div>
-      <div amp-next-page-more-box>
+      <div more-box>
           Here are a few more articles ...
       </div>  
     </amp-next-page>

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
@@ -258,7 +258,8 @@ <h2>Host page</h2>
         quis metus in mi pharetra tempus. Etiam dapibus tellus vitae blandit
         rhoncus. Fusce commodo risus id sapien ultrices vehicula.
       </p>
-    </div>
+    </div>    
+    <!-- amp-next-page configuration -->
     <amp-next-page>
       <script type="application/json">
         [
@@ -285,9 +286,9 @@ <h2>Host page</h2>
         ]
       </script>
       <div amp-next-page-separator>
-          Read more
+        Read more
       </div>
-      <div amp-next-page-more-box>
+      <div more-box>
           Here are a few more articles ...
       </div>  
     </amp-next-page>

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
@@ -61,7 +61,7 @@ <h2>Content discovery</h2>
         varius est suscipit vitae. Maecenas ut sapien diam. Vivamus viverra nisl
         at quam pellentesque posuere. Cras ut nibh non arcu dignissim elementum.
       </p>
-      <div class="hidden" amp-next-page-hide>I should be visible when loaded as a host page but hidden if loaded as a next-page</div>
+      <div class="hidden" next-page-hide>I should be visible when loaded as a host page but hidden if loaded as a next-page</div>
       <p>
         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 @@ <h2>Content discovery</h2>
         enim, non pellentesque sem. Nam non sem lacinia, lacinia nisi non,
         consectetur enim.
       </p>
-      <div class="sticky" amp-next-page-replace="my-sticky-element">I should get replaced by my sibling from each loaded page (page 0)</div>
-      <div class="fixed" amp-next-page-hide>I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 0)</div>
+      <div class="sticky" next-page-replace="my-sticky-element">I should get replaced by my sibling from each loaded page (page 0)</div>
+      <div class="fixed" next-page-hide>I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 0)</div>
       <p>
         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 @@ <h2>Content discovery</h2>
           }
         ]
       </script>
-      <div amp-next-page-separator>
+      <div separator>
           Read more
       </div>
-      <div amp-next-page-more-box>
+      <div more-box>
           Here are a few more articles ...
       </div>  
     </amp-next-page>

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
@@ -55,7 +55,7 @@ <h2>Content discovery</h2>
         varius est suscipit vitae. Maecenas ut sapien diam. Vivamus viverra nisl
         at quam pellentesque posuere. Cras ut nibh non arcu dignissim elementum.
       </p>
-      <div class="hidden" amp-next-page-hide>I should be visible when loaded as a host page but hidden if loaded as a next-page</div>
+      <div class="hidden" next-page-hide>I should be visible when loaded as a host page but hidden if loaded as a next-page</div>
       <p>
         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 @@ <h2>Content discovery</h2>
         enim, non pellentesque sem. Nam non sem lacinia, lacinia nisi non,
         consectetur enim.
       </p>
-      <div class="sticky" amp-next-page-replace="my-sticky-element">I got replaced by my sibling from page 1</div>
-      <div class="fixed" amp-next-page-hide>I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 1)</div>
+      <div class="sticky" next-page-replace="my-sticky-element">I got replaced by my sibling from page 1</div>
+      <div class="fixed" next-page-hide>I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 1)</div>
       <p>
         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
@@ -55,7 +55,7 @@ <h2>Content discovery</h2>
         varius est suscipit vitae. Maecenas ut sapien diam. Vivamus viverra nisl
         at quam pellentesque posuere. Cras ut nibh non arcu dignissim elementum.
       </p>
-      <div class="hidden" amp-next-page-hide>I should be visible when loaded as a host page but hidden if loaded as a next-page</div>
+      <div class="hidden" next-page-hide>I should be visible when loaded as a host page but hidden if loaded as a next-page</div>
       <p>
         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 @@ <h2>Content discovery</h2>
         enim, non pellentesque sem. Nam non sem lacinia, lacinia nisi non,
         consectetur enim.
       </p>
-      <div class="sticky" amp-next-page-replace="my-sticky-element">I got replaced by my sibling from page 2</div>
-      <div class="fixed" amp-next-page-hide>I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 2)</div>
+      <div class="sticky" next-page-replace="my-sticky-element">I got replaced by my sibling from page 2</div>
+      <div class="fixed" next-page-hide>I am fixed and should be visible when loaded as a host page but hidden if loaded as a next-page (page 2)</div>
       <p>
         Maecenas sed quam nec dolor rhoncus rutrum ut non mauris. Pellentesque
         ante dolor, pretium quis enim eu, condimentum volutpat augue. Donec nulla