amp-consent validator #14127
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"
zhouyx
force-pushed
the
amp-consent-validator
branch
2 times, most recently
from
d6b35ea
to
7c88e78
Compare
zhouyx
force-pushed
the
amp-consent-validator
branch
from
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. |
zhouyx
force-pushed
the
amp-consent-validator
branch
2 times, most recently
from
3898c30
to
39a49fe
Compare
zhouyx
force-pushed
the
amp-consent-validator
branch
from
39a49fe
to
9b8a6ec
Compare
|
@honeybadgerdontcare Thank you for the help! |
|
These validator changes will be live everywhere within an hour. |
For #13716