Skip to content

🐛Fix script tag validation issues with amp-experiment usage on amp-story#24909

Merged
gmajoulet merged 8 commits intoampproject:masterfrom
jackbsteinberg:amp-experiment-validator
Oct 10, 2019
Merged

🐛Fix script tag validation issues with amp-experiment usage on amp-story#24909
gmajoulet merged 8 commits intoampproject:masterfrom
jackbsteinberg:amp-experiment-validator

Conversation

@jackbsteinberg
Copy link
Copy Markdown
Contributor

Fixes a bug where the amp-validator would flag script tags used inside amp-experiment tags would flag the validator for [CUSTOM_JAVASCRIPT_DISALLOWED].
Resolves #24748

@amp-owners-bot amp-owners-bot bot requested a review from Enriqe October 4, 2019 17:56
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Oct 4, 2019

Hey @newmuis, these files were changed:

  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment-error.html
  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment-error.out
  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment.html
  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment.out
  • extensions/amp-story/validator-amp-story.protoascii

Hey @ampproject/wg-caching, these files were changed:

  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment-error.html
  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment-error.out
  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment.html
  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment.out
  • extensions/amp-story/validator-amp-story.protoascii

@honeybadgerdontcare
Copy link
Copy Markdown
Contributor

@banaag for review

@jackbsteinberg jackbsteinberg force-pushed the amp-experiment-validator branch from 636237e to 28deffb Compare October 7, 2019 17:19
@gmajoulet
Copy link
Copy Markdown
Contributor

Friendly ping @banaag

Copy link
Copy Markdown
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

Please add the license to the test files.

Copy link
Copy Markdown
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

validation looks good

@gmajoulet gmajoulet merged commit 75f8a22 into ampproject:master Oct 10, 2019
honeybadgerdontcare added a commit that referenced this pull request Oct 16, 2019
honeybadgerdontcare added a commit that referenced this pull request Oct 16, 2019
* cl/274603953 Revision bump for #24854

* cl/274639638 Revision bump for #24909
joshuarrrr pushed a commit to Parsely/amphtml that referenced this pull request Oct 22, 2019
* cl/274603953 Revision bump for ampproject#24854

* cl/274639638 Revision bump for ampproject#24909
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
…ory (ampproject#24909)

* Fix script tag validation issues with amp-experiment usage on amp-story

* Reformat comments and fix invalid amp-experiments

* Update outfile to reflect changes on comments and amp-experiments

* Add newline to ends of test files

* Add passing test for amp-story amp-experiment validator

* Add newlines to ends of outfiles

* Add license to test files

* Add license addition to validator test outfiles
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
* cl/274603953 Revision bump for ampproject#24854

* cl/274639638 Revision bump for ampproject#24909
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.

In amp-story, amp-experiment is alerted by amp validator, but it work correctly.

5 participants