-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[amp-list-load-more] documentation, demo, bugfix, and ui tweaks #19399
Conversation
bc6d0ee
to
1dabbf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some questions before LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you.
Merging master for the bundle size check issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sorry for the delayed review! Overall, looks really good! Just a it of confusion and a small nit.
@@ -281,6 +281,35 @@ We recommend using `binding="no"` or `binding="refresh"` for faster performance. | |||
|
|||
If `binding` attribute is not provided, default is `always`. | |||
|
|||
## Experimental: Infinite Scroll (amp-list-load-more) | |||
We've introduced an experiment called `amp-list-load-more` as an implementation for pagination and infinite scroll in amp-list. This is an experimental feature, and final APIs may change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove comma between "experimental feature, and final"
|
||
### Attributes | ||
#### load-more (mandatory) | ||
Adding this attribute will allow amp-list (with no value) to show a “load-more" button at the end of the amp-list. The value of this attribute can be set to “auto" to trigger automatic loading more elements three viewports down for an infinite scroll effect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind updating all mentions of amp-list
are surrounded in ``` and tag marks?
`<amp-list>` with the `load-more` attribute expects the following additional child elements: | ||
|
||
#### load-more-button (mandatory) | ||
An element containing the `load-more-button` attribute. Clicking on this button will trigger a fetch to load more elements from the url contained in the field of the data returned corresponding to the `load-more-bookmark` attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call it an element and a button. Should the element be <button>
or can it be any element used as a button? If there are only specific types of elements that can be used we should list them.
Part of #14060, #9423.
reloadCallback
, to implement button to reload last link after 404-ing.