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

Add canvas support in amp-script #23658

Merged
merged 12 commits into from
Aug 7, 2019
Merged

Add canvas support in amp-script #23658

merged 12 commits into from
Aug 7, 2019

Conversation

alabiaga
Copy link
Contributor

@alabiaga alabiaga commented Aug 2, 2019

Fixes: #23579

Validator changes to allow for canvas element in amp-script block

@jridgewell jridgewell requested review from Gregable, dreamofabear and alin04 and removed request for Gregable August 2, 2019 17:50
# CANVAS support.
tags: {
html_format: AMP
tag_name: "CANVAS"
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a standard HTML element, would you mind moving it to validator-main.protoascii under the appropriate HTML spec header?

You can leave the mandatory_ancestor thing here, and leave a comment describing where to look.

@alabiaga
Copy link
Contributor Author

alabiaga commented Aug 5, 2019

@choumx friendly ping. Want to make sure you were aware of this change.

@@ -3839,6 +3839,9 @@ tags: {
# 4.8 Links
# Links are a concept, rather than a tag. See tagspecs for "LINK", "A", etc.

# 4.8.11 The canvas element
# See extensions/amp-mustache/validator-amp-script.protoascii

Choose a reason for hiding this comment

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

I think Greg means moving the entire canvas tag spec here with a comment for mandatory_ancestor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right. He is also fine with this approach as long as one as point out in main where the rule is defined. Thanks

<h1>Example: hello-world.js</h1>
<amp-script layout=container src="/examples/amp-script/hello-world.js">
<h1>Example: example.js</h1>
<amp-script layout=container src="/examples/amp-script/example.js">

Choose a reason for hiding this comment

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

Can we name this something other than its grandparent folder name? Maybe amp-script-demo.js.

@alabiaga alabiaga merged commit 1ff7ef8 into ampproject:master Aug 7, 2019
@alabiaga alabiaga deleted the amp-script-canvas branch August 7, 2019 01:32
rsimha pushed a commit that referenced this pull request Aug 8, 2019
* cl/261776511 Add ValidationError::DEV_MODE_ONLY error code.

* cl/261804799 Improve amp-date-picker and amp-carousel validator errors.

* cl/261931393 Revision bump for #23598

* cl/262032455 Allow additional CSS features.

* cl/262149003 Allow double-spaces in <a rel> and <link rel>.

* cl/262178521 Revision bump for #23691

* cl/262193876 Revision bump for #23658

* cl/262223028 Revision bump for #23752

* cl/262230380 Revision bump for #23727

* Fix merge duplication mistakes.
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* add canvas support in amp-script

* new files

* file renamed

* address @Gregable comments

* fix test after rule changes

* fix test

* name change

* rename example.js to amp-script-demo.js

* update file from rename
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* cl/261776511 Add ValidationError::DEV_MODE_ONLY error code.

* cl/261804799 Improve amp-date-picker and amp-carousel validator errors.

* cl/261931393 Revision bump for ampproject#23598

* cl/262032455 Allow additional CSS features.

* cl/262149003 Allow double-spaces in <a rel> and <link rel>.

* cl/262178521 Revision bump for ampproject#23691

* cl/262193876 Revision bump for ampproject#23658

* cl/262223028 Revision bump for ampproject#23752

* cl/262230380 Revision bump for ampproject#23727

* Fix merge duplication mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canvas support in amp-script
4 participants