Skip to content
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

Merged
merged 17 commits into from
Nov 28, 2018

Conversation

cathyxz
Copy link
Contributor

@cathyxz cathyxz commented Nov 20, 2018

Part of #14060, #9423.

  1. Add more examples, in particular an endpoint that simulates pagination, # of items for load-end testing.
  2. Increase viewport preload constant from 1.5 to 3 viewports down.
  3. Minor bugfix regarding load failed state and loading state.
  4. Added reloadCallback, to implement button to reload last link after 404-ing.

@cathyxz cathyxz changed the title [WIP] Infinite scroll cleanup, examples, and bugfixes [amp-list-load-more] documentation, demo, bugfix, and ui tweaks Nov 21, 2018
extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved
extensions/amp-list/0.1/amp-list.js Show resolved Hide resolved
extensions/amp-list/0.1/amp-list.js Show resolved Hide resolved
Copy link
Contributor

@cvializ cvializ left a 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.

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved
build-system/app.js Outdated Show resolved Hide resolved
extensions/amp-list/amp-list.md Show resolved Hide resolved
extensions/amp-list/amp-list.md Outdated Show resolved Hide resolved
build-system/app.js Outdated Show resolved Hide resolved
build-system/app.js Outdated Show resolved Hide resolved
extensions/amp-list/0.1/amp-list.js Show resolved Hide resolved
extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved
Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@cathyxz
Copy link
Contributor Author

cathyxz commented Nov 28, 2018

Merging master for the bundle size check issue.

Copy link
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@CrystalOnScript CrystalOnScript left a 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.
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants