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.

Sign up for GitHub

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

Jump to bottom

amp-list load-more validation #20094

Merged
merged 5 commits into from Jan 14, 2019
Merged

amp-list load-more validation #20094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8696299
Add basic validation for amp-list without element customization
cathyxz Dec 27, 2018
d403d58
Add validation for amp-list children attributes
cathyxz Dec 27, 2018
8a1db50
Create tag_spec
cathyxz Jan 3, 2019
baab643
Remove reference points in favor of new tag
cathyxz Jan 10, 2019
499d05b
Add mandatory exclusion
cathyxz Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 43 additions & 0 deletions extensions/amp-list/0.1/test/validator-amp-list.html
View file
Open in desktop
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

valid test case with just load-more for the default case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

src="https://data.com/articles.json?ref=CANONICAL_URL"
load-more="auto"></amp-list>
</amp-list>
<!-- Valid amp-list with load-more="manual" attribute -->
<amp-list width=10 height=10
src="https://data.com/articles.json?ref=CANONICAL_URL"
load-more="manual"></amp-list>
</amp-list>
<!-- Valid amp-list with load-more and optional load-more-bookmark attribute -->
<amp-list width=10 height=10
src="https://data.com/articles.json?ref=CANONICAL_URL"
load-more="manual"
load-more-bookmark="next"></amp-list>
</amp-list>
<!-- Valid amp-list with custom children -->
<amp-list width=10 height=10
src="https://data.com/articles.json?ref=CANONICAL_URL"
load-more="manual"
load-more-bookmark="next">
<amp-list-load-more load-more-button></amp-list-load-more>
<amp-list-load-more load-more-loading></amp-list-load-more>
<amp-list-load-more load-more-failed></amp-list-load-more>
<amp-list-load-more load-more-end></amp-list-load-more>
</amp-list>
</amp-list>
<!-- Invalid: unsupported "binding" attribute value -->
<amp-list width=10 height=10
Expand All @@ -107,5 +133,22 @@
<amp-list src="https://data.com/articles.json?ref=CANONICAL_URL">
<div></div>
</amp-list>
<!-- Invalid amp-list with load-more attribute -->
<amp-list width=10 height=10
src="https://data.com/articles.json?ref=CANONICAL_URL"
load-more></amp-list>
</amp-list>
<!-- Invalid amp-list with bad load-more attribute -->
<amp-list width=10 height=10
src="https://data.com/articles.json?ref=CANONICAL_URL"
load-more="gibberish"
load-more-bookmark="next"></amp-list>
</amp-list>
<!-- Invalid load-more attributes -->
<div load-more-button></div>
<div load-more-loading></div>
<div load-more-failed></div>
<div load-more-end></div>
<amp-list-load-more></amp-list-load-more>
</body>
</html>
65 changes: 61 additions & 4 deletions extensions/amp-list/0.1/test/validator-amp-list.out
View file
Open in desktop
Expand Up @@ -91,34 +91,91 @@ amp-list/0.1/test/validator-amp-list.html:85:2 The attribute 'auto-resize' in ta
| 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
| src="https://data.com/articles.json?ref=CANONICAL_URL"
| load-more="auto"></amp-list>
| </amp-list>
| <!-- Valid amp-list with load-more="manual" attribute -->
| <amp-list width=10 height=10
| src="https://data.com/articles.json?ref=CANONICAL_URL"
| load-more="manual"></amp-list>
| </amp-list>
| <!-- Valid amp-list with load-more and optional load-more-bookmark attribute -->
| <amp-list width=10 height=10
| src="https://data.com/articles.json?ref=CANONICAL_URL"
| load-more="manual"
| load-more-bookmark="next"></amp-list>
| </amp-list>
| <!-- Valid amp-list with custom children -->
| <amp-list width=10 height=10
| src="https://data.com/articles.json?ref=CANONICAL_URL"
| load-more="manual"
| load-more-bookmark="next">
| <amp-list-load-more load-more-button></amp-list-load-more>
| <amp-list-load-more load-more-loading></amp-list-load-more>
| <amp-list-load-more load-more-failed></amp-list-load-more>
| <amp-list-load-more load-more-end></amp-list-load-more>
| </amp-list>
| </amp-list>
| <!-- Invalid: unsupported "binding" attribute value -->
| <amp-list width=10 height=10
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:91:2 The attribute 'binding' in tag 'amp-list' is set to the invalid value 'this-is-an-error'. (see https://www.ampproject.org/docs/reference/components/amp-list) [DISALLOWED_HTML]
amp-list/0.1/test/validator-amp-list.html:117:2 The attribute 'binding' in tag 'amp-list' is set to the invalid value 'this-is-an-error'. (see https://www.ampproject.org/docs/reference/components/amp-list) [DISALLOWED_HTML]
| src="https://data.com/articles.json?ref=CANONICAL_URL"
| binding="this-is-an-error">
| <div></div>
| </amp-list>
| <!-- Invalid: width is mistyped. -->
| <amp-list src="https://data.com/articles.json?ref=CANONICAL_URL"
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:97:2 The attribute 'wdith' may not appear in tag 'amp-list'. (see https://www.ampproject.org/docs/reference/components/amp-list) [DISALLOWED_HTML]
amp-list/0.1/test/validator-amp-list.html:123:2 The attribute 'wdith' may not appear in tag 'amp-list'. (see https://www.ampproject.org/docs/reference/components/amp-list) [DISALLOWED_HTML]
| wdith=10 height=10>
| <div></div>
| </amp-list>
| <!-- Invalid: missing at least src or [src]. -->
| <amp-list width=10 height=10>
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:102:2 The tag 'amp-list' is missing a mandatory attribute - pick at least one of ['src','[src]']. (see https://www.ampproject.org/docs/reference/components/amp-list) [AMP_TAG_PROBLEM]
amp-list/0.1/test/validator-amp-list.html:128:2 The tag 'amp-list' is missing a mandatory attribute - pick at least one of ['src','[src]']. (see https://www.ampproject.org/docs/reference/components/amp-list) [AMP_TAG_PROBLEM]
| <div></div>
| </amp-list>
| <!-- Invalid: width/height missing, so it's container layout
| which isn't supported. -->
| <amp-list src="https://data.com/articles.json?ref=CANONICAL_URL">
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:107:2 Incomplete layout attributes specified for tag 'amp-list'. For example, provide attributes 'width' and 'height'. (see https://www.ampproject.org/docs/reference/components/amp-list) [AMP_LAYOUT_PROBLEM]
amp-list/0.1/test/validator-amp-list.html:133:2 Incomplete layout attributes specified for tag 'amp-list'. For example, provide attributes 'width' and 'height'. (see https://www.ampproject.org/docs/reference/components/amp-list) [AMP_LAYOUT_PROBLEM]
| <div></div>
| </amp-list>
| <!-- Invalid amp-list with load-more attribute -->
| <amp-list width=10 height=10
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:137:2 The attribute 'load-more' in tag 'amp-list' is set to the invalid value ''. (see https://www.ampproject.org/docs/reference/components/amp-list) [DISALLOWED_HTML]
| src="https://data.com/articles.json?ref=CANONICAL_URL"
| load-more></amp-list>
| </amp-list>
| <!-- Invalid amp-list with bad load-more attribute -->
| <amp-list width=10 height=10
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:142:2 The attribute 'load-more' in tag 'amp-list' is set to the invalid value 'gibberish'. (see https://www.ampproject.org/docs/reference/components/amp-list) [DISALLOWED_HTML]
| src="https://data.com/articles.json?ref=CANONICAL_URL"
| load-more="gibberish"
| load-more-bookmark="next"></amp-list>
| </amp-list>
| <!-- Invalid load-more attributes -->
| <div load-more-button></div>
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:148:2 The attribute 'load-more-button' may not appear in tag 'div'. [DISALLOWED_HTML]
| <div load-more-loading></div>
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:149:2 The attribute 'load-more-loading' may not appear in tag 'div'. [DISALLOWED_HTML]
| <div load-more-failed></div>
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:150:2 The attribute 'load-more-failed' may not appear in tag 'div'. [DISALLOWED_HTML]
| <div load-more-end></div>
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:151:2 The attribute 'load-more-end' may not appear in tag 'div'. [DISALLOWED_HTML]
| <amp-list-load-more></amp-list-load-more>
>> ^~~~~~~~~
amp-list/0.1/test/validator-amp-list.html:152:2 The parent tag of tag 'amp-list-load-more' is 'body', but it can only be 'amp-list'. (see https://www.ampproject.org/docs/reference/components/amp-list) [AMP_TAG_PROBLEM]
| </body>
| </html>
40 changes: 40 additions & 0 deletions extensions/amp-list/validator-amp-list.protoascii
View file
Open in desktop
Expand Up @@ -55,6 +55,17 @@ tags: { # <amp-list> with mandatory src and/or [src] attr
}
attrs: { name: "credentials" }
attrs: { name: "items" }
attrs: {
name: "load-more"
value: "auto"
value: "manual"
}
attrs: {
name: "load-more-bookmark"
trigger: {
also_requires_attr: "load-more"
}
}
attrs: { name: "max-items" }
attrs: {
name: "reset-on-refresh"
Expand Down Expand Up @@ -94,6 +105,35 @@ tags: { # <amp-list> with mandatory src and/or [src] attr
supported_layouts: RESPONSIVE
}
}

tags: { # amp-list-load-more
html_format: AMP
html_format: EXPERIMENTAL
tag_name: "AMP-LIST-LOAD-MORE"
requires_extension: "amp-list"
mandatory_parent: "AMP-LIST"
attrs: {
name: "load-more-button"
value: ""
mandatory_oneof: "['load-more-button', 'load-more-failed', 'load-more-end', 'load-more-loading']"
}
attrs: {
name: "load-more-failed"
value: ""
mandatory_oneof: "['load-more-button', 'load-more-failed', 'load-more-end', 'load-more-loading']"
}
attrs: {
name: "load-more-loading"
value: ""
mandatory_oneof: "['load-more-button', 'load-more-failed', 'load-more-end', 'load-more-loading']"
}
attrs: {
name: "load-more-end"
value: ""
mandatory_oneof: "['load-more-button', 'load-more-failed', 'load-more-end', 'load-more-loading']"
}
}

# AMP4EMAIL disallows mustache in src attribute.
tags: { # <amp-list>
html_format: AMP4EMAIL
Expand Down