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

Built-in see-more for amp-list #13796

Merged
merged 7 commits into from Oct 19, 2018
Merged

Conversation

cpapazian
Copy link
Contributor

@cpapazian cpapazian commented Mar 4, 2018

Implements #13575

First pass. Outstanding issues/questions:

  • Whether to register position observer once or if current solution of clearing handler each time new page is fetched makes sense. I'm not familiar enough with performance characteristics of the position observer to determine which makes more sense /cc @aghassemi
  • Overflow onclick interaction with checkPendingChangeSize_
    onFocus event prevents additional overflow interactions from working reliably. see: Click events don't always fire from overflow div #13774
  • bikeshedding on names /cc @ericlindley-g
  • set classes on amp-list to indicate various loading states
  • tests and examples

@ericlindley-g
Copy link
Contributor

Sync'd w/ the group again, and the current naming proposed in the ITI looks great, so we can proceed with that version. Thanks!

@cpapazian
Copy link
Contributor Author

cpapazian commented Mar 5, 2018

@ericlindley-g wrt naming (and in the spirit of bikeshedding), since we're always providing a url to the next page in the response, I think it actually makes sense to swap things back a bit:

<amp-list load-more=<KeyInJSONForBookmarkValue> load-more-on-scroll>

or

<amp-list load-more=<KeyInJSONForBookmarkValue> load-more-auto>

The load-more attribute can default to next the same way items has a default expression.

@aghassemi
Copy link
Contributor

@cpapazian my 2 cents: I find load-more=auto more semantic and clear than load-more="some key" it is not very clear why the value of load-more is some key into JSON. load-more-bookmark="key" make that more clear IMO.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpapazian Thanks, this is a pretty good initial PR. Few requests also heads up that there is another biggish change to amp-list #13782 that will cause conflicts with this.

this.nextSrc_ = null;

/** @private {?float} */
this.pageOnScrollRatio_ = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some of the variable names may need to changed after bikeshedding, so I won't nit pick on them for now.

@@ -173,19 +188,23 @@ export class AmpList extends AMP.BaseElement {
* @return {!Promise}
* @private
*/
fetchList_() {
fetchList_(append) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsDoc for append. Since it is optional, let's name it opt_append so js doc would be {boolean=} opt_append

if (this.element.hasAttribute('single-item')) {
user().assert(typeof items !== 'undefined' ,
'Response must contain an array or object at "%s". %s',
itemsExpr, this.element);
if (!isArray(items)) {
items = [items];
}
} else if (this.element.hasAttribute('next-page')) {
const nextExpr = this.element.getAttribute('next-page');
this.nextSrc_ = getValueForExpr(result, nextExpr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to do assertHttpsUrl on nextSrc_.

Ideally we do a better error message if it is not specified, so a null assert before doing assertHttpsUrl would be nice. (e.g. user().assert(this.nextSrc_, "JSON response must specify a ....")

@@ -72,6 +81,9 @@ export class AmpList extends AMP.BaseElement {

/** @const @private {string} */
this.initialSrc_ = element.getAttribute('src');

/** @private {?string} */
this.nextSrc_ = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: loadMoreSrc_? helps make it a bit more clear what internal variables are tied to what feature.

if (this.element.hasAttribute('single-item')) {
user().assert(typeof items !== 'undefined' ,
'Response must contain an array or object at "%s". %s',
itemsExpr, this.element);
if (!isArray(items)) {
items = [items];
}
} else if (this.element.hasAttribute('load-more')) {
const nextExpr = this.element.getAttribute(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest expecting load-more-src as the default key. WDYT? (it is verbose, but having load-more as prefix for everything related to it creates an easier to understand API IMO)

}
});
}

setupNextPage_() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto naming: all nextPage -> loadMore

const overflow = this.getOverflowElement();
const triggerOnScroll = this.element.getAttribute('load-more') === 'auto';
if (!overflow && !triggerOnScroll) {
user().warn(TAG,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make this user.error() (also something we can prevent using validation rules)

overflow.onclick = null;
}

if (this.positionObserver_) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I don't see a benefit to turning this off just for the duration of the fetch.

}

this.element.setAttribute('src', this.nextSrc_);
this.nextSrc = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextSrc_

return batchFetchJsonFor(ampdoc, this.element, '.', policy);
}

maybeInstallPositionObserver_() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basic jsdoc (@private)

@cpapazian cpapazian force-pushed the amp-list-paging branch 3 times, most recently from 3f8f522 to 7b6003f Compare March 14, 2018 20:57
@cpapazian
Copy link
Contributor Author

cpapazian commented Mar 15, 2018

@aghassemi @choumx I'm just getting back to this. I fixed the naming and jsdoc over the weekend. I'm not an expert at jsdoc, so I added some explicit type annotations that may not be needed (the scheduleRender_(...) call and the return from getValueForExpr(...) for this.loadMoreSrc_. Let me know if there's a better way to implement these.

I also added a few tests, though I'm having trouble with the position observer test. It's working for me when I run in the browser, but not in the test case. Not sure if it has to do with how things are mocked, or something about how the promise chain resolves. Could use a little help here. Or maybe I don't need to bother with that one ... EDIT: looks like the promise chain was broken, since this.getVsync().measure didn't return a promise ... replacing with measurePromise fixes the sequencing.

Other outstanding issues:

  • Not clear on this.renderItems_ should be handled in append state. Is it possible that this gets overwritten and a page is list? Should I append to the this.renderItems_.items? Should it be converted back to a queue, as it was in previous version? Or is there some other mechanism that guarantees things aren't lost?
  • Still TODO is adding load-more-loading and load-more-failed divs as detailed in the UX notes .

Thanks!

@aghassemi
Copy link
Contributor

@cpapazian I believe appending to this.renderItems_.items would be correct. Things will and should get overridden when amp-bind changes the src or refresh action is called (where this.renderItems_ is recreated). But that's expected. For example if src changes in the case of server-side sorting, well all the extra loaded items based on infinite scrolling would go away naturally and they would need to be loaded again.

@cpapazian cpapazian force-pushed the amp-list-paging branch 3 times, most recently from 0ab89dc to d37ae0a Compare March 17, 2018 05:09
@cpapazian
Copy link
Contributor Author

cpapazian commented Mar 19, 2018

@aghassemi I think this is getting close. Just a few outstanding questions ...

  • Where should the new CSS for the loading and failed indicators go? Should I create an amp-list CSS file? Also, how do the class names look? I'm not quite sure on naming convention here.
  • Not quite sure on the use of this.getVsync().mutate(.... Do I need to wrap toggle of the overylay version of the loading indicator?
  • Let me know if any tests need to be added or cleaned up. Was thinking the loading indicator test should be a little more precise--I wanted to test for presence of the specific css classes, but had some trouble with expectFetchAndRender and where to stub out the fetches, so it just spies on the toggle function.
  • Click events don't always fire from overflow div #13774 still seems to be outstanding. It would be good to test this once a fix has landed.

thanks!

@aghassemi
Copy link
Contributor

@cpapazian
1- For CSS, I assume most of the needed CSS is just to have the new divs initially be hidden and toggling happens in JS, right? In that case, we actually want them in amp.css since if we put them in amp-list.css, they get loaded too late (because the whole extension, including its CSS are lazy-loaded) but we want those initial visibility rules to be applied as soon as page is visible. In the CSS, let's make sure they are pre-qualified with amp-list selector so we don't apply the rule too broadly.

2- Yes, essentially any DOM manipulation that does not happen synchronously during buildCallback and layoutCallback need to be in a mutate (vsync` is AMP's version of FastDOM, it batches and performs all measures and mutates together to eliminate layout thrashing)

3A- Before writing tests, we also need to put an experiment flag around this feature (as we do with most new features in AMP). This involves adding a new entry in experiments.js and then using isExperimentOn in amp-list to only allow the new feature when the experiment is on. This will help few interested parties to experiment with the feature first and when we are confident all is good, to release to public for wider adoption.

3B- Regarding testing strategy, ideally existing tests just pass without modifications, then we can add a separate describe to group load-more tests all together, this would allow you to mock differently than the existing test setup and also to enable the experimentFlag (via toggleExperiment) just for this portion of the tests.

4- Agreed.

@cpapazian
Copy link
Contributor Author

Having some trouble with tests after rebasing. It looks like changing the measure call to measurePromise in rendered()_ breaks some assumptions in test. I made that change so I could align the promise chain and assert side effects of the setLoadMore_ call. without measurePromise, the promise chain in doRenderPass_ doesn't seem to line up ... anything below the rendered_ call will happen immediately.

I had initially set this up this way to avoid a race condition between the overflow logic in the parent and setLoadMore_. Though if I am making assumptions about how the render pass logic works, I can look at other ways ...

@@ -252,9 +254,6 @@ describes.realWin('amp-list component', {
element.setAttribute('src', 'https://new.com/list.json');
list.mutatedAttributesCallback({'src': 'https://new.com/list.json'});
});
listMock.expects('toggleLoading').withExactArgs(false).once();
listMock.expects('togglePlaceholder').withExactArgs(false).once();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these listMocks were set twice: once in expectFetchAndRender and once in the test itself, but they are only called once. when rendered_() returns a promise (measurePromise), these conditions fail.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great!

css/amp.css Outdated
@@ -543,6 +543,23 @@ i-amphtml-scroll-container.amp-active, i-amp-scroll-container.amp-active {
visibility: visible;
}

amp-list [overflow] > .i-amphtml-loader {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may break existing cases where another amp-element exists inside inside overflow by hiding their loading icon.

I believe should should do two things (for this and the roles below)

1- do amp-list[load-more] as the prefix to control all this behaviour to load more.
2- use direct child selector for .i-amphtml-loader so we don't hide other loading indicators from whatever else might be in [overflow]

this.loadMoreSrc_ = null;

/** @private {?Element|undefined} */
this.loadMoreOverflow_ = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we normally do null instead of undefined as default then type can be just {?Element}

if (this.loadMoreOverflow_) {
this.loadMoreOverflow_.classList.toggle(
'amp-visible', !!this.loadMoreSrc_);
this.loadMoreOverflow_.onclick = () => this.loadMoreCallback_();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listen(elem, 'click', cb) would be preferred (listen can be imported from event-helper.js)

* @private
*/
getLoadMoreLoadingElement_() {
if (this.loadMoreLoadingElement_ === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see that you are using the undefined vs null for lazy querying. Honestly I don't mind this happening in buildCallback (if isLoadMoreEnabled) rather than doing it lazily (buildCallback runs during prerender mode and is meant for querying things you may need later anyway)

@@ -316,6 +313,112 @@ describes.realWin('amp-list component', {
});
});

describe('load more', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another type of test to consider if these start to become too fragile/hacky, is integration tests using describes.integration. For those tests, essentially you provide some valid AMP HTML/CSS and they run as if it is a really AMP page, then you can use normal DOM API to wait for stuff and verify. So more blackbox testing closer to something you would normally do with WebDriver. Nothing can be mocked.

If you search for describes.integration you can find some good examples. The test-position-observer one for example actually scrolls the page and then asserts stuff.

with auto load-more
@aghassemi aghassemi self-assigned this May 30, 2018
@nainar
Copy link
Contributor

nainar commented Aug 9, 2018

@cpapazian is this something you are working on actively? To reduce the stale PRs I will close this for now since there hasn't been action on this in 4 months. Please reopen this if you intend to work on this.

@nainar nainar closed this Aug 9, 2018
@aghassemi
Copy link
Contributor

@cpapazian @nainar I've been the hold up for this PR.
@cpapazian some updates: We have prioritized infinite scroll support for amp-list this quarter and @cathyxz will be working on seeing it through. I am reopening the PR as we will rebase and merge this first before continuing work on infinite list.

@aghassemi aghassemi reopened this Aug 9, 2018
@cathyxz cathyxz self-assigned this Aug 21, 2018
@cathyxz
Copy link
Contributor

cathyxz commented Aug 21, 2018

Rebasing and testing this.

@cathyxz
Copy link
Contributor

cathyxz commented Oct 17, 2018

@aghassemi can you please review this again? Merged master and fixed the overflow element issue by using a new overflow element instead of the existing one. I've tested and regression tested most of amp-list except for the SSR template changes from @alabiaga on which I have very little context. I've fixed the relevant tests, but I'm not 100% confident that I didn't break anything for SSR. @alabiaga could you review this as well? Would appreciate it if you could test the SSR functionality or give guidance on testing. =)

@cathyxz cathyxz assigned alabiaga and dreamofabear and unassigned cathyxz Oct 17, 2018
@alabiaga
Copy link
Contributor

@cathyxz this feature should just remain unsupported for SSR until we decide whether we want to enable this for AMP4Email which SSR is mainly for. So we are safe there were no changes to load more in the condition that ssr is enabled. Code changes in ssr path looks good to me.

@cathyxz
Copy link
Contributor

cathyxz commented Oct 18, 2018

Yup, you're absolutely correct on that end. This is not a feature for SSR, and we just need to know that it doesn't accidentally break SSR. Thanks a lot @alabiaga !

@@ -137,24 +137,24 @@ describes.realWin('amp-list component', {
});

it('should fetch and render', () => {
const items = [{title: 'Title1'}];
const fetched = {'items': [{title: 'Title1'}]};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is a function of how the test works, and does not imply a change in code logic, but a second opinion here would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine to me. I can see how this is the result of fetch_ not taking expr anymore and the implications of it with the mocking. Also looks "more correct" than the old version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the sanity check! I pulled this out to a constant so that in case we ever have to change this again, we don't have to type it 30 times in this file.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had reviewed this before and focused on the new changes, testing and looking for regression potentials. LGTM.

* @private
*/
fetch_(opt_refresh = false, opt_itemsExpr) {
const expr = opt_itemsExpr || '.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any place where fetch is called with opt_itemsExpr set

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This is possibly an error. Let me look into it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this is actually legacy code that was made unnecessary because we pulled out the call to getValueForExpr(data, itemsExpr) and put that in the parent in fetchList_ per:

fetch = this.fetch_(opt_refresh).then(data => {
        let items = getValueForExpr(data, itemsExpr);

I think this is actually okay and we can safely remove the opt_itemsExpr parameter (which I did).

@@ -137,24 +137,24 @@ describes.realWin('amp-list component', {
});

it('should fetch and render', () => {
const items = [{title: 'Title1'}];
const fetched = {'items': [{title: 'Title1'}]};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine to me. I can see how this is the result of fetch_ not taking expr anymore and the implications of it with the mocking. Also looks "more correct" than the old version.

// If there's nothing currently being rendered, schedule a render pass.
if (!this.renderItems_) {
this.renderPass_.schedule();
}
this.renderItems_ = {data, resolver, rejecter};

this.renderItems_ = {data, opt_append, resolver, rejecter};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitL it's not really opt at this point, just append would do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you!

@cathyxz cathyxz merged commit 6658fa3 into ampproject:master Oct 19, 2018
visibility: hidden;
}

amp-list[load-more] > [load-more-loading].amp-visible,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, alphabetize

amp-list[load-more] > [load-more-button].amp-visible,
amp-list[load-more] > [load-more-failed].amp-visible,
amp-list[load-more] > [load-more-loading].amp-visible,

here and below

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
* implement paging for amp-list

with auto load-more

* Address comments

* Pull out literals to constants

* Fix missed constant conversion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants