amp-bind: Support changing `src` on amp-list #8735

kmh287 · 2017-04-12T18:26:18Z

This PR introduces changes to allow binding to src on amp-list. When the value of the src attribute changes, the amp-list element performs another XHR to get new list data and replaces the current data with the rendered results of the XHR.

Reading order:

Changes to amp-list
- amp-list.js for implementation changes
- test-amp-list.js for updated tests
- validator-amp-list.protoascii for validator changes
Changes to bind
- bind-validator.js for adding a new rule for amp-list and its src attribute
- test-bind-integration.js for integration test changes
- amp-bind-integrations.html for integration test fixture changes
Bind example
- app.js To create JSON endpoints for two sets of list data
- list.html For an example of amp-list with a changeable src
Updated Documentation
- amp-bind.md To document addition of amp-list and its src property

Partial for #8700

/to @choumx

dreamofabear · 2017-04-12T22:45:50Z

examples/bind/list.html

+      <strong>Unit Price</strong>: ${{unitPrice}}
+    </template>
+  </amp-list>
+</body>


Can you build a paginated sorting/filtering demo as a template for ampbyexample?

sure, let's discuss more offline

dreamofabear · 2017-04-12T22:46:20Z

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

+  /** @override */
+  mutatedAttributesCallback(mutations) {
+    const srcMutation = mutations['src'];
+    if (srcMutation) {


mutations['src'] !== undefined

dreamofabear · 2017-04-12T22:46:53Z

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

@@ -82,6 +114,11 @@ export class AmpList extends AMP.BaseElement {
      }
    });
  }
+
+  /** @return {Promise} */


@visibleForTesting

kmh287 · 2017-04-14T18:12:42Z

/to @aghassemi

aghassemi · 2017-04-14T22:42:22Z

examples/bind/list.html

+  <button on="tap:AMP.setState({listSrc:'/list/fruit-data/get'})">Show Fruit</button>
+  <button on="tap:AMP.setState({listSrc:'/list/vegetable-data/get'})">Show Vegetables</button>
+
+  <amp-list layout="responsive" src="/list/fruit-data/get" [src]="listSrc || '/list/fruit-data/get'" width="600" height="600">


why is the || '/list/fruit-data/get' part needed here?

We're encouraging developers to ensure that the expression for [property] initially evaluates to the value for property. This example is simple enough that there's no risk of unexpected behavior when bind first evaluates the expression, but it doesn't hurt to include the OR.

Got it. Thanks for the explanation.

aghassemi · 2017-04-14T22:46:50Z

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

+    if (srcMutation != undefined) {
+      const p = this.populateList_();
+      if (getMode().test) {
+        this.mutationPromise_ = p;


Can we instead try to get what we need by stubbing this.populateList_ in the test instead`?

100% agree that this code isn't ideal. I try to avoid depending on private properties/methods when writing unit tests, hence the promise and the visibleForTesting getter. If you think that changing it as proposed will make this easier to read / work with let me know and I'll make the change. 👍

Unit tests accessing private properties/methods is definitely fine, I prefer that over exporting specific methods/properties just for testing.

aghassemi · 2017-04-14T22:49:34Z

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

@@ -66,6 +97,7 @@ export class AmpList extends AMP.BaseElement {
   * @private
   */
  rendered_(elements) {
+    removeChildren(this.container_);


since the list can now become very dynamic, can you please add setAttribute('aria-live', 'polite'); to this.element in the buildCallback. That would let screen readers announce changes when list refreshes.

Good idea, and that's on the element, not the container, right?

Doesn't matter for screen-readers, element is probably better since it would allow page authors to override it (maybe they want aria-live='assertive' for certain lists).

aghassemi

lgtm

dreamofabear · 2017-04-20T00:18:50Z

extensions/amp-bind/amp-bind.md

@@ -283,6 +283,7 @@ Only binding to the following components and attributes are allowed:
 | `<amp-carousel type=slides>` | `[slide]`<sup>1</sup> | Changes the currently displayed slide index. [See an example](https://ampbyexample.com/advanced/image_galleries_with_amp-carousel/#linking-carousels-with-amp-bind).
 | `<amp-iframe>` | `[src]` | Changes the iframe's source URL. |
 | `<amp-img>` | `[alt]`<br>`[attribution]`<br>`[src]`<br>`[srcset]` | See corresponding [amp-img attributes](https://www.ampproject.org/docs/reference/components/media/amp-img#attributes). |
+| `<amp-list>` | `[src]` | Change the list's source URL. |


How about "Fetches JSON from the new URL and re-renders while discarding old content."

Tweaked it a bit but otherwise like the parallelism with amp-state's description 👍

dreamofabear · 2017-04-20T00:19:16Z

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

+
+  /** @override */
+  mutatedAttributesCallback(mutations) {
+    const srcMutation = mutations['src'];


Nit: This can be inlined below.

kmh287

Thanks for the review!

kmh287 · 2017-04-21T14:09:28Z

extensions/amp-bind/amp-bind.md

@@ -283,6 +283,7 @@ Only binding to the following components and attributes are allowed:
 | `<amp-carousel type=slides>` | `[slide]`<sup>1</sup> | Changes the currently displayed slide index. [See an example](https://ampbyexample.com/advanced/image_galleries_with_amp-carousel/#linking-carousels-with-amp-bind).
 | `<amp-iframe>` | `[src]` | Changes the iframe's source URL. |
 | `<amp-img>` | `[alt]`<br>`[attribution]`<br>`[src]`<br>`[srcset]` | See corresponding [amp-img attributes](https://www.ampproject.org/docs/reference/components/media/amp-img#attributes). |
+| `<amp-list>` | `[src]` | Change the list's source URL. |


Tweaked it a bit but otherwise like the parallelism with amp-state's description 👍

kmh287 · 2017-04-21T14:09:56Z

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

+
+  /** @override */
+  mutatedAttributesCallback(mutations) {
+    const srcMutation = mutations['src'];


kmh287 added the PR use: Work in Progress label Apr 12, 2017

kmh287 requested a review from dreamofabear April 12, 2017 19:57

kmh287 removed the PR use: Work in Progress label Apr 12, 2017

dreamofabear reviewed Apr 12, 2017

View reviewed changes

kmh287 requested a review from aghassemi April 14, 2017 17:55

kmh287 force-pushed the bind_amp_list_src branch from b6dd0f0 to 3b07ecb Compare April 14, 2017 17:57

aghassemi reviewed Apr 14, 2017

View reviewed changes

aghassemi approved these changes Apr 17, 2017

View reviewed changes

dreamofabear approved these changes Apr 20, 2017

View reviewed changes

kmh287 and others added 15 commits April 21, 2017 10:17

change live list to support mutations to its src attribute

5560af5

bind-validator updates and some comments on amp-list

20aca85

typos

071f4d5

documentation update

83b88c3

add test for changing src attribute

2f850e7

support dynamic elements in amp-list

e9c6cd6

amp validaotr support

23fc56c

merge

b73ae86

integration tests and some cleanup

4d43e14

typo

528c634

pr comments

3fa7d90

aria-live

e933f59

ali's comment

cb51dcc

cleanup

be26175

pr comments

4407f20

kmh287 force-pushed the bind_amp_list_src branch from b2df967 to 4407f20 Compare April 21, 2017 14:30

cleanup

449b7ba

kmh287 commented Apr 21, 2017

View reviewed changes

kmh287 merged commit 18733f7 into ampproject:master Apr 21, 2017

karsten-eger mentioned this pull request Apr 25, 2017

FR: event to trigger/prevent loading for amp-list #8931

Closed

DarXector pushed a commit to DarXector/amphtml that referenced this pull request Apr 25, 2017

amp-bind: Support changing src on amp-list (ampproject#8735)

a80d353

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

amp-bind: Support changing src on amp-list (ampproject#8735)

dd80cbe

ampprojectbot added the PR use: In Beta / Experimental label May 1, 2017

KenneyE pushed a commit to spotxchange/amphtml that referenced this pull request May 3, 2017

amp-bind: Support changing src on amp-list (ampproject#8735)

ad95e4d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

amp-bind: Support changing `src` on amp-list #8735

amp-bind: Support changing `src` on amp-list #8735

kmh287 commented Apr 12, 2017 •

edited

dreamofabear Apr 12, 2017

kmh287 Apr 13, 2017

dreamofabear Apr 12, 2017

kmh287 Apr 13, 2017

dreamofabear Apr 12, 2017

kmh287 Apr 13, 2017

kmh287 commented Apr 14, 2017

aghassemi Apr 14, 2017

kmh287 Apr 17, 2017 •

edited

aghassemi Apr 17, 2017

aghassemi Apr 14, 2017

kmh287 Apr 17, 2017

aghassemi Apr 17, 2017

kmh287 Apr 17, 2017

aghassemi Apr 14, 2017

kmh287 Apr 17, 2017 •

edited

aghassemi Apr 17, 2017

kmh287 Apr 17, 2017

aghassemi left a comment

dreamofabear Apr 20, 2017

kmh287 Apr 21, 2017

dreamofabear Apr 20, 2017

kmh287 Apr 21, 2017

kmh287 left a comment

kmh287 Apr 21, 2017

kmh287 Apr 21, 2017

amp-bind: Support changing src on amp-list #8735

amp-bind: Support changing src on amp-list #8735

Conversation

kmh287 commented Apr 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 commented Apr 14, 2017

Choose a reason for hiding this comment

kmh287 Apr 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 Apr 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amp-bind: Support changing `src` on amp-list #8735

amp-bind: Support changing `src` on amp-list #8735

kmh287 commented Apr 12, 2017 •

edited

kmh287 Apr 17, 2017 •

edited

kmh287 Apr 17, 2017 •

edited