✨ Implement the refresh action for amp-list#13725
✨ Implement the refresh action for amp-list#13725dreamofabear merged 4 commits intoampproject:masterfrom
Conversation
extensions/amp-list/0.1/amp-list.js
Outdated
|
|
||
| this.registerAction('refresh', () => { | ||
| this.fetchList_(); | ||
| }, ActionTrust.LOW); |
There was a problem hiding this comment.
Curious why low trust? Do you need this to trigger on scroll?
There was a problem hiding this comment.
No, I don't need it to trigger on scroll. I just figured there wasn't any reason to lock it down too tightly since its not really dangerous (like a form submit or something). I can change it to ActionTrust.HIGH if you want though. It doesn't really matter to me.
There was a problem hiding this comment.
Let's change to hight trust. There are two issues with low-trust:
1- Resizing logic allows amp-list to resize if it is "active" so a low-trust event from a child of amp-list can refresh and cause relayout which low-trust actions should not be able to do.
2- Even if relayout was not an issue, I like to not allow low-trust actions to trigger network requests.
There was a problem hiding this comment.
Ok. I changed it to ActionTrust.HIGH now.
extensions/amp-list/0.1/amp-list.js
Outdated
|
|
||
| this.registerAction('refresh', () => { | ||
| this.fetchList_(); | ||
| }, ActionTrust.LOW); |
There was a problem hiding this comment.
Let's change to hight trust. There are two issues with low-trust:
1- Resizing logic allows amp-list to resize if it is "active" so a low-trust event from a child of amp-list can refresh and cause relayout which low-trust actions should not be able to do.
2- Even if relayout was not an issue, I like to not allow low-trust actions to trigger network requests.
|
|
||
| ### Refreshing data | ||
|
|
||
| The `amp-list` element exposes a `refresh` action that other elements can reference in `on="tap:..."` attributes. |
There was a problem hiding this comment.
Could you please add this action to the master list of actions and events in https://github.com/ampproject/amphtml/blob/master/spec/amp-actions-and-events.md
extensions/amp-list/0.1/amp-list.js
Outdated
| this.initialSrc_ = element.getAttribute('src'); | ||
|
|
||
| this.registerAction('refresh', () => { | ||
| this.fetchList_(); |
There was a problem hiding this comment.
we should wrap this in if(this.layoutCompleted_){} I don't think this action should be allowed to do anything before amp-list is laidout (e.g. be used for initial load). If we need to support that use-case, there is a bit more we need to do than just call fetchList_
* Implement the refresh action for amp-list (ampproject#13717) * Resolve comments from Will and Ali
cc @aghassemi @choumx