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.

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

Validator rules/test for amp-action-macro #20928

Merged
merged 25 commits into from
Feb 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ec3d00b
toggle maybeChangeHeight in layers after remeasures to indicate to ru…
alabiaga Feb 5, 2019
c43effd
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 7, 2019
c55d693
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 7, 2019
6684255
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 7, 2019
3b66bd8
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 8, 2019
4b649d8
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 11, 2019
b05e6cc
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 12, 2019
e2c10e9
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 12, 2019
05f1fc4
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 13, 2019
48dfd0d
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 13, 2019
73cd238
revert this change from master
alabiaga Feb 13, 2019
ed326e1
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 15, 2019
2894879
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 19, 2019
ab40c63
validator rules/tests amp-action-macro
alabiaga Feb 19, 2019
c69d36e
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 19, 2019
2c9dbde
merge
alabiaga Feb 19, 2019
1b1bac7
revert files
alabiaga Feb 19, 2019
a7dcc2f
address honeybadgerdontcare@ comments
alabiaga Feb 19, 2019
0f126e2
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 20, 2019
40fcbf1
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 20, 2019
94d6168
Merge branch 'master' into amp-action-macro-validator-rules
alabiaga Feb 20, 2019
3178389
choumx comments
alabiaga Feb 20, 2019
beb8a70
Merge branch 'master' of https://github.com/ampproject/amphtml
alabiaga Feb 21, 2019
edf55aa
Merge branch 'master' into amp-action-macro-validator-rules
alabiaga Feb 21, 2019
dc76975
fix test for id attr
alabiaga Feb 21, 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
41 changes: 41 additions & 0 deletions extensions/amp-action-macro/validator-amp-action-macro.protoascii
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#
# Copyright 2019 The AMP HTML Authors. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS-IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the license.
#
tags: { # amp-action-macro
html_format: AMP
Copy link
Contributor

Choose a reason for hiding this comment

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

have all these formats approved enabling this extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

Let's start with AMP and ACTIONS conservatively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

html_format: ACTIONS
tag_name: "SCRIPT"
extension_spec: {
name: "amp-action-macro"
version: "0.1"
version: "latest"
}
attr_lists: "common-extension-attrs"
}
tags: { # <amp-action-macro>
html_format: AMP
html_format: ACTIONS
tag_name: "AMP-ACTION-MACRO"
requires_extension: "amp-action-macro"
attrs: {
name: "arguments"
}
attrs: {
name: "execute"
mandatory: true
}
attr_lists: "mandatory-id-attr"
spec_url: "https://www.ampproject.org/docs/reference/components/amp-action-macro"
}
54 changes: 54 additions & 0 deletions validator/testdata/feature_tests/amp-action-macro.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<!--
Copyright 2019 The AMP HTML Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS-IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the license.
-->
<!doctype html>
<html ⚡>
<head>
<meta charset="utf-8">
<link rel="canonical" href="./regular-html-version.html" />
<meta name="viewport" content="width=device-width,minimum-scale=1">
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<script async custom-element="amp-action-macro" src="https://cdn.ampproject.org/v0/amp-action-macro-0.1.js"></script>
<script async src="https://cdn.ampproject.org/v0.js"></script>
<style amp-custom>
.red {
color: red;
}
.green {
color: green;
}
</style>
</head>
<body>

<!-- Valid -->
<amp-action-macro id="action-macro-id" execute="amp-component.doSomething()">

Choose a reason for hiding this comment

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

We renamed this attribute to execute? Isn't it a bit confusing with the registered action being "execute" as well?

Copy link
Contributor Author

@alabiaga alabiaga Feb 20, 2019

Choose a reason for hiding this comment

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

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>

</amp-action-macro>

<!-- Valid, optional arguments -->
<amp-action-macro id="action-macro-id" execute="amp-component.doSomething(foo=x,bar=y)"
arguments="x,y">
</amp-action-macro>

<!-- Invalid, requires ID -->
<amp-action-macro execute="amp-component.doSomething()">
</amp-action-macro>

<!-- Invalid, requires execute -->
<amp-action-macro id="action-macro-id">
</amp-action-macro>

</body>
</html>
60 changes: 60 additions & 0 deletions validator/testdata/feature_tests/amp-action-macro.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
FAIL
| <!--
| Copyright 2019 The AMP HTML Authors. All Rights Reserved.
|
| Licensed under the Apache License, Version 2.0 (the "License");
| you may not use this file except in compliance with the License.
| You may obtain a copy of the License at
|
| http://www.apache.org/licenses/LICENSE-2.0
|
| Unless required by applicable law or agreed to in writing, software
| distributed under the License is distributed on an "AS-IS" BASIS,
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
| See the License for the specific language governing permissions and
| limitations under the license.
| -->
| <!doctype html>
| <html ⚡>
| <head>
| <meta charset="utf-8">
| <link rel="canonical" href="./regular-html-version.html" />
| <meta name="viewport" content="width=device-width,minimum-scale=1">
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
| <script async custom-element="amp-action-macro" src="https://cdn.ampproject.org/v0/amp-action-macro-0.1.js"></script>
| <script async src="https://cdn.ampproject.org/v0.js"></script>
| <style amp-custom>
| .red {
| color: red;
| }
| .green {
| color: green;
| }
| </style>
| </head>
| <body>
|
| <!-- Valid -->
| <amp-action-macro id="action-macro-id" execute="amp-component.doSomething()">
| </amp-action-macro>
|
| <!-- Valid, optional arguments -->
| <amp-action-macro id="action-macro-id" execute="amp-component.doSomething(foo=x,bar=y)"
| arguments="x,y">
| </amp-action-macro>
|
| <!-- Invalid, requires ID -->
| <amp-action-macro execute="amp-component.doSomething()">
>> ^~~~~~~~~
feature_tests/amp-action-macro.html:46:2 The mandatory attribute 'id' is missing in tag 'amp-action-macro'. (see https://www.ampproject.org/docs/reference/components/amp-action-macro) [AMP_TAG_PROBLEM]
| </amp-action-macro>
|
| <!-- Invalid, requires execute -->
| <amp-action-macro id="action-macro-id">
>> ^~~~~~~~~
feature_tests/amp-action-macro.html:50:2 The mandatory attribute 'execute' is missing in tag 'amp-action-macro'. (see https://www.ampproject.org/docs/reference/components/amp-action-macro) [AMP_TAG_PROBLEM]
| </amp-action-macro>
|
| </body>
| </html>