✨Built-in see-more for amp-list by cpapazian · Pull Request #13796 · ampproject/amphtml

cpapazian · 2018-03-04T21:15:30Z

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 · 2018-03-05T22:46:36Z

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 · 2018-03-05T23:01:51Z

@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 · 2018-03-05T23:43:04Z

@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.

aghassemi

@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.

aghassemi · 2018-03-05T21:51:57Z

extensions/amp-list/0.1/amp-list.js

+    this.nextSrc_ = null;
+
+    /** @private {?float} */
+    this.pageOnScrollRatio_ = null;


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

aghassemi · 2018-03-05T21:52:33Z

extensions/amp-list/0.1/amp-list.js

   * @private
   */
-  fetchList_() {
+  fetchList_(append) {


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

aghassemi · 2018-03-05T22:00:46Z

extensions/amp-list/0.1/amp-list.js

        }
+      } else if (this.element.hasAttribute('next-page')) {
+        const nextExpr = this.element.getAttribute('next-page');
+        this.nextSrc_ = getValueForExpr(result, nextExpr);


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 ....")

aghassemi · 2018-03-08T02:48:17Z

extensions/amp-list/0.1/amp-list.js

    this.initialSrc_ = element.getAttribute('src');
+
+    /** @private {?string} */
+    this.nextSrc_ = null;


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

aghassemi · 2018-03-08T02:53:05Z

extensions/amp-list/0.1/amp-list.js

          items = [items];
        }
+      } else if (this.element.hasAttribute('load-more')) {
+        const nextExpr = this.element.getAttribute(


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)

aghassemi · 2018-03-08T02:56:51Z

extensions/amp-list/0.1/amp-list.js

    });
  }

+  setupNextPage_() {


ditto naming: all nextPage -> loadMore

aghassemi · 2018-03-08T02:58:07Z

extensions/amp-list/0.1/amp-list.js

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


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

aghassemi · 2018-03-08T03:06:01Z

extensions/amp-list/0.1/amp-list.js

+      overflow.onclick = null;
+    }
+
+    if (this.positionObserver_) {


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

aghassemi · 2018-03-08T03:06:29Z

extensions/amp-list/0.1/amp-list.js

+    }
+
+    this.element.setAttribute('src', this.nextSrc_);
+    this.nextSrc = null;


aghassemi · 2018-03-08T03:06:47Z

extensions/amp-list/0.1/amp-list.js

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


basic jsdoc (@private)

cpapazian · 2018-03-15T02:17:18Z

@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 · 2018-03-17T03:25:18Z

@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 · 2018-03-19T04:25:44Z

@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 · 2018-03-19T17:16:54Z

@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 · 2018-03-20T16:41:50Z

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 ...

cpapazian · 2018-03-20T20:56:29Z

extensions/amp-list/0.1/test/test-amp-list.js

      list.mutatedAttributesCallback({'src': 'https://new.com/list.json'});
    });
-    listMock.expects('toggleLoading').withExactArgs(false).once();
-    listMock.expects('togglePlaceholder').withExactArgs(false).once();


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.

aghassemi

This is looking great!

aghassemi · 2018-03-21T21:54:46Z

css/amp.css

  visibility: visible;
 }

+amp-list [overflow] > .i-amphtml-loader {


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]

aghassemi · 2018-03-21T23:16:01Z

extensions/amp-list/0.1/amp-list.js

+    this.loadMoreSrc_ = null;
+
+    /** @private {?Element|undefined} */
+    this.loadMoreOverflow_ = undefined;


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

aghassemi · 2018-03-21T23:24:54Z

extensions/amp-list/0.1/amp-list.js

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


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

aghassemi · 2018-03-21T23:31:43Z

extensions/amp-list/0.1/amp-list.js

+   * @private
+   */
+  getLoadMoreLoadingElement_() {
+    if (this.loadMoreLoadingElement_ === undefined) {


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)

aghassemi · 2018-03-21T23:38:34Z

extensions/amp-list/0.1/test/test-amp-list.js

    });
  });

+  describe('load 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

…paging

nainar · 2018-08-09T18:26:24Z

@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.

aghassemi · 2018-08-09T18:29:13Z

@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.

cathyxz · 2018-08-21T21:51:15Z

Rebasing and testing this.

cathyxz · 2018-10-17T20:53:29Z

@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. =)

alabiaga · 2018-10-18T14:33:28Z

@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 · 2018-10-18T17:49:49Z

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 !

cathyxz · 2018-10-18T18:08:09Z

extensions/amp-list/0.1/test/test-amp-list.js


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


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.

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.

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.

aghassemi

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

aghassemi · 2018-10-18T22:24:01Z

extensions/amp-list/0.1/amp-list.js

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


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

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

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).

aghassemi · 2018-10-18T22:48:17Z

extensions/amp-list/0.1/test/test-amp-list.js


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


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.

aghassemi · 2018-10-18T22:50:56Z

extensions/amp-list/0.1/amp-list.js

    }
-    this.renderItems_ = {data, resolver, rejecter};
+
+    this.renderItems_ = {data, opt_append, resolver, rejecter};


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

Done. Thank you!

…paging

alabiaga · 2018-10-26T17:52:45Z

css/amp.css

+  visibility: hidden;
+}
+
+amp-list[load-more] > [load-more-loading].amp-visible,


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

* implement paging for amp-list with auto load-more * Address comments * Pull out literals to constants * Fix missed constant conversion

googlebot added the cla: yes label Mar 4, 2018

aghassemi requested review from aghassemi and dreamofabear March 5, 2018 04:48

aghassemi reviewed Mar 8, 2018

View reviewed changes

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

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

cpapazian force-pushed the amp-list-paging branch from aae9082 to 9d21c27 Compare March 20, 2018 06:59

cpapazian commented Mar 20, 2018

View reviewed changes

aghassemi reviewed Mar 21, 2018

View reviewed changes

cpapazian force-pushed the amp-list-paging branch from 526160f to 36318ad Compare March 25, 2018 17:16

cpapazian force-pushed the amp-list-paging branch 2 times, most recently from 63c1ed9 to 9a6ec8f Compare April 1, 2018 17:46

implement paging for amp-list

5b995a2

with auto load-more

cpapazian force-pushed the amp-list-paging branch from 9a6ec8f to 5b995a2 Compare April 1, 2018 20:40

Merge branch 'master' of github.com:ampproject/amphtml into amp-list-…

0a6855f

…paging

aghassemi self-assigned this May 30, 2018

nainar closed this Aug 9, 2018

aghassemi reopened this Aug 9, 2018

cathyxz self-assigned this Aug 21, 2018

cathyxz force-pushed the amp-list-paging branch from 8f1a0cd to 43b4d1a Compare October 17, 2018 20:50

cathyxz requested review from aghassemi and alabiaga October 17, 2018 20:50

cathyxz assigned alabiaga and dreamofabear and unassigned cathyxz Oct 17, 2018

cathyxz force-pushed the amp-list-paging branch from 43b4d1a to 57003dc Compare October 18, 2018 17:47

Merge branch 'master' into amp-list-paging

92a8a39

cathyxz force-pushed the amp-list-paging branch from 57003dc to 92a8a39 Compare October 18, 2018 17:51

cathyxz reviewed Oct 18, 2018

View reviewed changes

aghassemi approved these changes Oct 18, 2018

View reviewed changes

cathyxz added 4 commits October 18, 2018 16:21

Address comments

32cbdcc

Pull out literals to constants

12fabcb

Merge branch 'master' of github.com:ampproject/amphtml into amp-list-…

0d6da96

…paging

Fix missed constant conversion

709097e

cathyxz merged commit 6658fa3 into ampproject:master Oct 19, 2018

alabiaga reviewed Oct 26, 2018

View reviewed changes

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018

✨Built-in see-more for amp-list (ampproject#13796)

698776d

* implement paging for amp-list with auto load-more * Address comments * Pull out literals to constants * Fix missed constant conversion

Conversation

cpapazian commented Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericlindley-g commented Mar 5, 2018

Uh oh!

cpapazian commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aghassemi commented Mar 5, 2018

Uh oh!

aghassemi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpapazian commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aghassemi commented Mar 17, 2018

Uh oh!

cpapazian commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aghassemi commented Mar 19, 2018

Uh oh!

cpapazian commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nainar commented Aug 9, 2018

Uh oh!

aghassemi commented Aug 9, 2018

Uh oh!

cathyxz commented Aug 21, 2018

Uh oh!

cathyxz commented Oct 17, 2018

Uh oh!

alabiaga commented Oct 18, 2018

Uh oh!

cathyxz commented Oct 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpapazian commented Mar 4, 2018 •

edited

Loading

cpapazian commented Mar 5, 2018 •

edited

Loading

aghassemi left a comment •

edited

Loading

cpapazian commented Mar 15, 2018 •

edited

Loading

cpapazian commented Mar 19, 2018 •

edited

Loading