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 minor renaming and fixes #26468

Merged
merged 28 commits into from
Jan 28, 2020
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 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
e9dea23
Preserve only name changes and fixes
wassgha Jan 24, 2020
22c840a
Suggested fixes
wassgha Jan 24, 2020
7077068
Merge branch 'master' into amp-next-page-renaming
wassgha Jan 27, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions extensions/amp-next-page/1.0/amp-next-page.css
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
6 changes: 5 additions & 1 deletion extensions/amp-next-page/1.0/page.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand Down Expand Up @@ -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()) {
Expand Down
37 changes: 17 additions & 20 deletions extensions/amp-next-page/1.0/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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) {
wassgha marked this conversation as resolved.
Show resolved Hide resolved
// TODO(wassgha): Append a "load next article" button?
return null;
}
Expand Down Expand Up @@ -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 */)
);
}
Expand All @@ -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 (
Expand Down Expand Up @@ -727,10 +730,7 @@ export class NextPageService {
* @private
*/
getSeparatorElement_(element) {
const providedSeparator = childElementByAttr(
element,
'amp-next-page-separator'
);
const providedSeparator = childElementByAttr(element, 'separator');
wassgha marked this conversation as resolved.
Show resolved Hide resolved
// TODO(wassgha): Use templates (amp-mustache) to render the separator
if (providedSeparator) {
removeElement(providedSeparator);
Expand All @@ -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;
}

Expand All @@ -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);
Expand All @@ -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;
}
}
6 changes: 3 additions & 3 deletions extensions/amp-next-page/1.0/test/test-amp-next-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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>
Expand Down
7 changes: 4 additions & 3 deletions test/manual/amp-next-page/1.0/amp-next-page.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -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">
[
Expand All @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down