-
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 element skeleton. #948
Conversation
Also added URL substitutions. |
} | ||
} | ||
|
||
AMP.registerElement('amp-list', AmpList); |
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, do we really need to special case lists?
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.
You mean, do we need an amp-list
vs a generic amp-render
? If so, here are the reasons why I thought this was useful:
- It's actually more direct with the original request
- Clear how to extend it to an infinite scroll case (someone mentioned that as well)
- Semantics of changing height are more clear
- I expect as follow up we can add similar strategy to
amp-carousel
and such
PTAL |
const assert = AMP.assert; | ||
|
||
|
||
/** |
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.
Add something
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.
Done
LGTM |
Partial for #657.
This PR essentially brings
amp-list
andamp-mustache
elements to a functional state. The "mustache" experiment has to be on to try it.Some of the key outstanding questions here:
/cc @erwinmombay