Validator rules/test for amp-action-macro #20928
Conversation
…ntime that doc height could have changed
| # limitations under the license. | ||
| # | ||
| tags: { # amp-action-macro | ||
| html_format: AMP |
There was a problem hiding this comment.
have all these formats approved enabling this extension?
There was a problem hiding this comment.
no, good point though the extension augments and improves the dev experience with using actions so perhaps it's ok? But perhaps it's safer to exclude first and just add if it's requested.
@choumx thoughts on this?
There was a problem hiding this comment.
Let's start with AMP and ACTIONS conservatively.
| name: "amp-action-macro" | ||
| version: "0.1" | ||
| version: "latest" | ||
| requires_usage: GRANDFATHERED |
| version: "0.1" | ||
| version: "latest" | ||
| requires_usage: GRANDFATHERED | ||
| deprecated_allow_duplicates: true |
| html_format: ACTIONS | ||
| tag_name: "AMP-ACTION-MACRO" | ||
| requires_extension: "amp-action-macro" | ||
| attrs: { name: "id" mandatory: true } |
There was a problem hiding this comment.
please break each field onto their own lines when there are more than one. e.g.
attrs: {
name: "id"
mandatory: true
}
| requires_extension: "amp-action-macro" | ||
| attrs: { name: "id" mandatory: true } | ||
| attrs: { name: "execute" mandatory: true } | ||
| attrs: { name: "arguments" mandatory: false } |
There was a problem hiding this comment.
please alphabetize by attrs.name, e.g. arguments first, then execute, then id.
also mandatory defaults to false so unnecessary to include it.
| <amp-action-macro id="action-macro-id"> | ||
| </amp-action-macro> | ||
|
|
||
| <!-- Valid, optional arguments --> |
There was a problem hiding this comment.
please put valid tests before invalid tests.
| # limitations under the license. | ||
| # | ||
| tags: { # amp-action-macro | ||
| html_format: AMP |
There was a problem hiding this comment.
no, good point though the extension augments and improves the dev experience with using actions so perhaps it's ok? But perhaps it's safer to exclude first and just add if it's requested.
@choumx thoughts on this?
| name: "amp-action-macro" | ||
| version: "0.1" | ||
| version: "latest" | ||
| requires_usage: GRANDFATHERED |
| version: "0.1" | ||
| version: "latest" | ||
| requires_usage: GRANDFATHERED | ||
| deprecated_allow_duplicates: true |
| html_format: ACTIONS | ||
| tag_name: "AMP-ACTION-MACRO" | ||
| requires_extension: "amp-action-macro" | ||
| attrs: { name: "id" mandatory: true } |
| requires_extension: "amp-action-macro" | ||
| attrs: { name: "id" mandatory: true } | ||
| attrs: { name: "execute" mandatory: true } | ||
| attrs: { name: "arguments" mandatory: false } |
| <amp-action-macro id="action-macro-id"> | ||
| </amp-action-macro> | ||
|
|
||
| <!-- Valid, optional arguments --> |
| <body> | ||
|
|
||
| <!-- Valid --> | ||
| <amp-action-macro id="action-macro-id" execute="amp-component.doSomething()"> |
There was a problem hiding this comment.
We renamed this attribute to execute? Isn't it a bit confusing with the registered action being "execute" as well?
There was a problem hiding this comment.
Yes, reason being that it clashed with the action attribute that is also allowed and present in a <form>.
I don't think it's confusing. Now there is a 1 to 1 mapping between the caller that calls execute method and the action being invoked in the action macro, which is now associated with the execute attribute.
Example:
<amp-action-macro
id="scrollable-carousel-macro-refers-an-action-macro"
execute="scrollable-carousel.slideTo(index=x)"
arguments="x">
</amp-action-macro>
<button on="tap:scrollable-carousel-macro-args.execute(x=1)">Go to slide 1</button>
|
@choumx friendly ping |
* cl/234896906 Revision bump for #20969 * cl/235090073 Allow i-amphtml-layout on AMP elements only for Transformed AMP * cl/235216325 Disallow document properties in <form> attribute name. * cl/235271919 Revision bump for #20989 * cl/235833553 Revision bump for #20928 * cl/235843207 Revision bump for #20905
* cl/234896906 Revision bump for ampproject#20969 * cl/235090073 Allow i-amphtml-layout on AMP elements only for Transformed AMP * cl/235216325 Disallow document properties in <form> attribute name. * cl/235271919 Revision bump for ampproject#20989 * cl/235833553 Revision bump for ampproject#20928 * cl/235843207 Revision bump for ampproject#20905
* cl/234896906 Revision bump for ampproject#20969 * cl/235090073 Allow i-amphtml-layout on AMP elements only for Transformed AMP * cl/235216325 Disallow document properties in <form> attribute name. * cl/235271919 Revision bump for ampproject#20989 * cl/235833553 Revision bump for ampproject#20928 * cl/235843207 Revision bump for ampproject#20905
#19494