Show loading indicator and placeholder when an amp-list refetches by josh313 · Pull Request #13782 · ampproject/amphtml

josh313 · 2018-03-02T22:53:59Z

Shows the loading indicator and placeholder when an amp-list refetches due to an amp-bind src change or the refresh action (✨ Implement the refresh action for amp-list #13725)
Resolves amp-list should show loading indicator and placeholder during refetch #13718

…mp-list-loading

aghassemi · 2018-03-05T21:09:25Z

src/custom-element.js

        return;
      }

+      if (state && !this.implementation_.doesReuseLoadingIndicator() &&


any reason this block moved down?

Not that I remember. I'm not sure how this got moved. I moved it back now.

aghassemi · 2018-03-05T21:11:02Z

src/base-element.js

+  }
+
+  /**
+   * Returns is reused again after the render.


Returns whether loading indicator is reused...

aghassemi · 2018-03-05T21:13:06Z

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

  }

+  /** @override */
+  doesReuseLoadingIndicator() {


I like to suggest renaming to isLoadingReused. doesprefix is not commonly used in AMP and other related loading-related methods just say loading instead of loadingIndicator

aghassemi · 2018-03-05T21:13:42Z

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

      throw user().createError('Error fetching amp-list', error);
-    });
+    })
+        .then(() => {


nit: no new line needed here

aghassemi · 2018-03-05T21:14:42Z

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

    }
  }

+  /** @override */


a bit of comment to clarify what's happening here, something along the lines of amp-list reuses the loading indicator when list is fetched again via bind mutation or refresh action

aghassemi · 2018-03-05T21:16:36Z

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

    if (!this.element.getAttribute('src')) {
      return Promise.resolve();
    }
+    this.togglePlaceholder(true);


This feature is technically is breaking UI change and I can see cases where one may not want this feature at all. I recommend putting this behind an attribute show-loading-on-refresh or similar. I will bring it up in our design review tomorrow morning to see what others think.

Makes sense. Let me know the outcome of the design review and I'll update the code accordingly.

Added to #13718 (comment).

tl;dr: reset-on-refresh attribute needed as opt-in signal to get this behaviour.

…mp-list-loading

…te is present

…mp-list-loading

aghassemi

Thanks for making the changes. Few small comments and we should be good to go after.

aghassemi · 2018-03-09T23:15:32Z

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

+        // Hide in case fallback was displayed for a previous fetch.
+        this.toggleFallbackInMutate_(false);
+      }
+      this.togglePlaceholder(false);


this is a change from previous code. These should happen automatically anyway. I recommend keeping the old code structure to eliminate any regressions e.g.

const fetch =... fetch.then() { if(fallback) etc... } return fetch;

You mean move this .then(...) block back to layoutCallback()? That doesn't work because then the loading and placeholder don't go away after a reload (either from the refresh action or an amp-bind change). I just tried it now.

aghassemi · 2018-03-09T23:17:15Z

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

+        this.toggleFallbackInMutate_(true);
+        this.togglePlaceholder(false);
+      } else {
+        throw error;


ditto, if we stick with the old code structure, this should not be needed.

aghassemi · 2018-03-09T23:20:59Z

extensions/amp-list/validator-amp-list.protoascii

  attrs: { name: "items" }
  attrs: { name: "max-items" }
  attrs: { name: "single-item" }
+  attrs: { name: "reset-on-refresh" }


please add value="" that would ensure reset-on-refresh can not be given a value and is treated as a binary flag.
also alphabetical ordering of the attributes names

…mp-list-loading

aghassemi · 2018-03-13T17:53:21Z

@josh313 PR LGTM, I am gonna do some manual edge case testing and then merge it.

aghassemi · 2018-03-13T18:16:02Z

@josh313 Meanwhile, could you please take a look at the failing test?

josh313 · 2018-03-13T21:18:21Z

@aghassemi Thanks. I fixed the unit tests now. The integration tests appear to have just stalled. I'm guessing that's unrelated to my change. Is there a way to restart it?

…mp-list-loading

aghassemi · 2018-03-14T19:52:10Z

@josh313 failure was due some whitespace nits in the validator test file. added a fix, they should pass now. I will let you know when I finish testing and merging this, will happen this week for sure.

josh313 · 2018-03-14T20:16:24Z

@aghassemi Sounds great. Thanks!

aghassemi · 2018-03-20T04:08:14Z

Rebased and did some local testing, looks good. will merge and do more testing on production sites with Canary that's cut tomorrow.

Update: (3/21)
Tests on Canary with few live sites for regression, all looks good.

yajiv185 · 2018-03-20T11:59:46Z

@aghassemi From when we will be able to use this?

aghassemi · 2018-03-20T16:30:33Z

@yajiv185 It will be in dev channel tomorrow so you can play with the feature, having your AMP pages validate with this feature is ~ 2-3 weeks away.

* Allow meta http-equiv=x-dns-prefetch-control * Introduce excludes tagspec feature * Revision bump for #13782 * Revision bump for #14040 * Revision bump for #14157,#14127

Gregable · 2018-03-22T17:35:51Z

These validator changes will be live everywhere within an hour.

Show loading indicator and placeholder when an amp-list refetches

4fedcf6

googlebot added the cla: yes label Mar 2, 2018

Merge branch 'master' of josh313.github.com:ampproject/amphtml into a…

6dc787b

…mp-list-loading

dreamofabear requested review from aghassemi and dreamofabear March 5, 2018 20:45

aghassemi reviewed Mar 5, 2018

View reviewed changes

josh313 added 6 commits March 5, 2018 16:07

Merge branch 'master' of josh313.github.com:ampproject/amphtml into a…

1b895f9

…mp-list-loading

Resolve Ali's comments

c04d0f0

Fix bug with loading indicator showing on relayout for amp-list

43f6dea

Only reset and show loading/placeholder when reset-on-refresh attribu…

5d056ca

…te is present

Merge branch 'master' of josh313.github.com:ampproject/amphtml into a…

3609c3a

…mp-list-loading

Fix tests to work with the new reset-on-refresh attribute

0c5fd6c

aghassemi reviewed Mar 9, 2018

View reviewed changes

josh313 added 3 commits March 12, 2018 13:41

Update validator according to Ali's comment

efc24a1

Fix lint errors

9edae2f

Merge branch 'master' of josh313.github.com:ampproject/amphtml into a…

185a77b

…mp-list-loading

Fix test

84a8356

josh313 and others added 2 commits March 14, 2018 11:20

Merge branch 'master' of josh313.github.com:ampproject/amphtml into a…

a46c993

…mp-list-loading

Update validator-amp-list.out

455ce17

aghassemi added 4 commits March 19, 2018 20:03

Merge branch 'master' into amp-list-loading

d4f561b

Update validator-amp-list.out

6a6369f

Update validator-amp-list.out

3c80d97

Update validator-amp-list.out

14301c0

aghassemi mentioned this pull request Mar 20, 2018

amp-list: document new reset-on-refresh attribute #14128

Merged

aghassemi approved these changes Mar 20, 2018

View reviewed changes

aghassemi merged commit 0c73103 into ampproject:master Mar 20, 2018

Gregable pushed a commit that referenced this pull request Mar 22, 2018

Revision bump for #13782

effcfc1

Gregable added a commit that referenced this pull request Mar 22, 2018

Validator Roll Up (#14202)

a0e7a22

* Allow meta http-equiv=x-dns-prefetch-control * Introduce excludes tagspec feature * Revision bump for #13782 * Revision bump for #14040 * Revision bump for #14157,#14127

jridgewell mentioned this pull request Apr 30, 2018

toggleLoading() does not honor isLoadingEnabled() #14972

Closed

aghassemi mentioned this pull request May 1, 2018

Ensure toggleLoading() respects isLoadingEnabled() #14982

Merged

Conversation

josh313 commented Mar 2, 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!

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!

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!

aghassemi commented Mar 13, 2018

Uh oh!

aghassemi commented Mar 13, 2018

Uh oh!

josh313 commented Mar 13, 2018

Uh oh!

aghassemi commented Mar 14, 2018

Uh oh!

josh313 commented Mar 14, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

aghassemi commented Mar 20, 2018

Uh oh!

Gregable commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aghassemi commented Mar 20, 2018 •

edited

Loading

yajiv185 commented Mar 20, 2018 •

edited

Loading