amp-consent validator#14127
Conversation
| name: "amp-consent" | ||
| allowed_versions: "0.1" | ||
| allowed_versions: "latest" | ||
| requires_usage: GRANDFATHERED |
There was a problem hiding this comment.
New extensions should not be grandfathered. Please remove.
|
|
||
| tags: { # amp-consent | ||
| html_format: AMP | ||
| html_format: EXPERIMENTAL |
There was a problem hiding this comment.
I believe this may be unintended and that this extension shouldn't be a part of EXPERIMENTAL format. (same for tags below)
There was a problem hiding this comment.
I saw a lot of tags are using the EXPERIMENTAL format : ) removed.
|
Please also include a test file (.html and .out). |
| } | ||
| attr_lists: "common-extension-attrs" | ||
| } | ||
| tags: { # amp-consent (json) |
There was a problem hiding this comment.
Since <amp-consent> is unique, should this also be unique or can runtime handle multiple json scripts as children of <amp-consent>.
There was a problem hiding this comment.
One question: how can I specify that there should be at least one and only one json script? Is unique enough? Note <amp-consent> could have multiple other child nodes.
There was a problem hiding this comment.
Can any of the other multiple child nodes be a <script type="application/json>? If not then unique: true is what you want.
There was a problem hiding this comment.
If this is to be tied expilcitly to <amp-consent> tag and only one exists then it needs:
satisfies: "amp-consent extension .json script" and unique: true.
Then <amp-consent> tag would need:
requires: "amp-consent extension .json script"
d6b35ea to
7c88e78
Compare
7c88e78 to
c5e3d21
Compare
| | | ||
| | <amp-consent id='ABC'> | ||
| >> ^~~~~~~~~ | ||
| amp-consent/0.1/test/validator-amp-consent-invalid.html:33:0 The implied layout 'CONTAINER' is not supported by tag 'amp-consent'. (see https://www.ampproject.org/docs/reference/components/amp-consent) [AMP_LAYOUT_PROBLEM] |
There was a problem hiding this comment.
I am surprised that no error against no json script was thrown here. Any ideas?
There was a problem hiding this comment.
That's because there is a json script present in the document in a correct location under an . I'd remove that second one, add the proper layout and also add a comment about what is being tested (e.g. <!-- Valid -->, <!-- Invalid: no json script tag -->
|
@honeybadgerdontcare I added the test. However I could not get the tests passed even after copying everything from the error message. Any ideas? |
|
@zhouyx I think that is due to spaces in the license section. It looks like in travis that the blank lines have one space for each blank line in the .out file but it should be two spaces. I'd remove the blank lines between the html contents but obviously not for the license. |
3898c30 to
39a49fe
Compare
39a49fe to
9b8a6ec
Compare
|
@honeybadgerdontcare Thank you for the help! |
|
These validator changes will be live everywhere within an hour. |
For #13716