amp-list load-more validation #20094
Conversation
cathyxz
force-pushed
the
validation/list
branch
from
fd83620
to
c108935
Compare
| @@ -94,6 +105,33 @@ tags: { # <amp-list> with mandatory src and/or [src] attr | |||
| supported_layouts: RESPONSIVE | |||
| } | |||
| } | |||
| tags: { # amp-list load-more children | |||
There was a problem hiding this comment.
Basically amp-list with load-more should be allowed to have up to 4 div children, each containing one of these attributes. These attributes should be mutually exclusive, and there should only be at most one div containing each attribute. If there's a better way to "validate" that, I'd love to know. Otherwise, I'll just write some JS asserts in the source.
There was a problem hiding this comment.
I think the alternative here is to define a reference point for amp list [load-more] and have that be a mandatory parent, but I'm not sure where we are on the usage of reference points discussion (the last I heard we are refactoring / changing the way we use them?).
There was a problem hiding this comment.
Actually, per our conversation in #19967, it's highly likely that we would like to allow this to be any element, not just a <div>. Is there better way to validate this without whitelisting all the allowed tags here, or introducing reference points?
There was a problem hiding this comment.
You could add these attributes to $GLOBALS and they would be allowed on any element. Then requires_extension could be added to them to ensure that AMP-LIST is included as a script tag. However, the requirement of having the parent be amp-list is then lost as that's not possible to put in an attribute spec since it really only applies to tags. Further they could then also be put on the <amp-list> element and I'm not sure what that would even mean.
There was a problem hiding this comment.
If it does move to being on any element I'd suggest making the attribute name more explicit about amp-list, e.g. amp-list-load-more-button.
There was a problem hiding this comment.
That wasn't my suggestion. I suggested making a new tag spec with the tag being amp-list-load-more.
There was a problem hiding this comment.
Apologies I misunderstood. Please allow me to clarify: as in literally creating a new tag called <amp-list-load-more>?
There was a problem hiding this comment.
Awesome. That does seem like the ideal solution. Especially since that will also let us cleanly whitelist which children we want to allow in these elements. Thanks a lot @honeybadgerdontcare for the offline followup as well!
cathyxz
requested review from
Gregable,
powdercloud,
honeybadgerdontcare and
aghassemi
and removed request for
powdercloud
| @@ -86,6 +86,32 @@ | |||
| src="https://data.com/articles.json?ref=CANONICAL_URL" | |||
| auto-resize> | |||
| <div></div> | |||
| <!-- Valid amp-list with load-more="auto" attribute --> | |||
| <amp-list width=10 height=10 | |||
There was a problem hiding this comment.
valid test case with just load-more for the default case?
There was a problem hiding this comment.
So even though I have not programmatically enforced this, I think the conclusion of the discussion on https://github.com/ampproject/amphtml/pull/19967/files#diff-4154928399fad29a55f4c6450302eef6R300 is that we should only have auto or manual, and remove the default option to avoid confusion. @CrystalFaith @andrewwatterson please keep me honest. xD
cathyxz
force-pushed
the
validation/list
branch
from
4c1f0c3
to
8a1db50
Compare
|
@honeybadgerdontcare made the changes you requested. Could you PTAL? =) |
cathyxz
changed the title
|
I see the reference points, is that the final form? Didn't you want to make the attributes unique on an element (e.g.
|
|
Oops, this is embarrassing. I jumped the gun a little. My push failed on the pre-submit check. |
* cl/229240741 Allow target attribute on form[method=POST] for ACTIONS * cl/229285760 Revision bump for #20094 * cl/229285815 Revision bump for #20322 * cl/229408305 Validate "transformer" type identifier value. * cl/229425442 Revision bump for #20330 * cl/229435530 Include version of transformers used in ValidationResult
* Add basic validation for amp-list without element customization * Add validation for amp-list children attributes * Create tag_spec * Remove reference points in favor of new tag * Add mandatory exclusion
* cl/229240741 Allow target attribute on form[method=POST] for ACTIONS * cl/229285760 Revision bump for ampproject#20094 * cl/229285815 Revision bump for ampproject#20322 * cl/229408305 Validate "transformer" type identifier value. * cl/229425442 Revision bump for ampproject#20330 * cl/229435530 Include version of transformers used in ValidationResult
DO NOT MERGE THIS UNTIL INFINITE SCROLL IS READY TO LAUNCH.The documentation for
amp-list load-moreis #19967.load-moreandload-more-bookmarkas attributes onamp-list.amp-listto contain elements containingload-more-loading,load-more-failed,load-more-button,load-more-end.To Reviewers: please take a look at the comment regarding the children with attributes and let me know if there's a better way or more precise validation rule that we should use.