Skip to content

Suggestion for an <amp-embed> builtin tag#1607

Merged
cramforce merged 1 commit intoampproject:masterfrom
taboola:Taboola-amp-embed-suggestion
Jan 28, 2016
Merged

Suggestion for an <amp-embed> builtin tag#1607
cramforce merged 1 commit intoampproject:masterfrom
taboola:Taboola-amp-embed-suggestion

Conversation

@nitzanvolman
Copy link
Copy Markdown

@nitzanvolman nitzanvolman commented Jan 27, 2016

relates to issue #1527
amp-embed aims to solve the use case of embeddable elements that have similar functional requirements as amp-ad, but it would not be semantically correct to use amp-ad. (e.g. organic content recommendations)

the suggestion is to implement amp-embed as alias for amp-ad

cramforce
cramforce reviewed Jan 27, 2016
View reviewed changes
src/custom-element.js Outdated
Copy link
Copy Markdown
Member

@cramforce cramforce Jan 27, 2016

Choose a reason for hiding this comment

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

Do we need the state right now?

Copy link
Copy Markdown
Author

@nitzanvolman nitzanvolman Jan 27, 2016

Choose a reason for hiding this comment

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

We are not using it in this iteration, thought it might be useful to keep.

Copy link
Copy Markdown
Member

@cramforce cramforce Jan 27, 2016

Choose a reason for hiding this comment

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

Lets get rid of it for now.

@cramforce cramforce self-assigned this Jan 27, 2016
@cramforce
Copy link
Copy Markdown
Member

cramforce commented Jan 27, 2016

@powdercloud Could you take a look at the validator changes?

@cramforce
Copy link
Copy Markdown
Member

cramforce commented Jan 27, 2016

@nitzanvolman Please add one usage to the ads.amp.html example. For now this will require updating the validator integration test whitelist here https://github.com/ampproject/amphtml/blob/master/test/integration/test-example-validation.js#L65

@googlebot
Copy link
Copy Markdown

googlebot commented Jan 27, 2016

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@nitzanvolman nitzanvolman force-pushed the Taboola-amp-embed-suggestion branch from 71290a1 to 8f2ae06 Compare January 28, 2016 14:31
@nitzanvolman
Copy link
Copy Markdown
Author

nitzanvolman commented Jan 28, 2016

@cramforce
Updated the branch with the latest comments,
please take a look at the validator white list -> seems like the previous one regexp was failing, also it seems like a new validation error related to brightcove popped up (was it something we did, or is this an external change?).
also - can you take a look at the changes to test-amp-ad.js and give your blessing? (which now tests both amp-embed and amp-ad)

@cramforce
Copy link
Copy Markdown
Member

cramforce commented Jan 28, 2016

@nitzanvolman The validation error for brightcove was just fixed (This is actually the point of the test. It found an actual error :)

Will take another look

cramforce
cramforce reviewed Jan 28, 2016
View reviewed changes
test/integration/test-example-validation.js Outdated
Copy link
Copy Markdown
Member

@cramforce cramforce Jan 28, 2016

Choose a reason for hiding this comment

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

Rebase and then make another entry (2nd regex) for your specific error.

@cramforce
Copy link
Copy Markdown
Member

cramforce commented Jan 28, 2016

@nitzanvolman Just some tiny comments. Please rebase and squash commits.

@Gregable @powdercloud Could you take a look at the validator change. amp-embed should validate like amp-ad.

cramforce
cramforce reviewed Jan 28, 2016
View reviewed changes
validator/validator.protoascii Outdated
Copy link
Copy Markdown
Member

@cramforce cramforce Jan 28, 2016

Choose a reason for hiding this comment

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

Please add a comment here and with amp-ad that changes should be reflected in the other respectively.

Copy link
Copy Markdown
Author

@nitzanvolman nitzanvolman Jan 28, 2016

Choose a reason for hiding this comment

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

@powdercloud since i will be removing shortly this from this PR as per your instructions, i think this comment above should be directed to you.
should add a comment that future changes on amp-ad should be done on amp-embed and vice versa.

Copy link
Copy Markdown
Contributor

@powdercloud powdercloud Jan 28, 2016

Choose a reason for hiding this comment

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

Yes thank you I did that - this is the (merged) commit:
679e684

@powdercloud
Copy link
Copy Markdown
Contributor

powdercloud commented Jan 28, 2016

For the validator.protoascii, I'd like to grab it and apply it internally, then propagate it to github; so the best way in this change is to leave it out. Reason being, we need to rev the revision id and if we make changes in both places we'll get conflicts, so it is more convenient that way. It looks fine in spirit though, and thanks much for understanding.
Update: This part has happened, so you can undo the validator.protoascii change in your PR and you should get the equivalent from our master branch.

@nitzanvolman nitzanvolman force-pushed the Taboola-amp-embed-suggestion branch 2 times, most recently from 13aebe2 to 0f58b55 Compare January 28, 2016 20:25
added 'element alias' registration, implemented amp-embed tag, modifi…
893e58c 
…ed test-amp-ad to test agains amp-embed as well
@nitzanvolman nitzanvolman force-pushed the Taboola-amp-embed-suggestion branch from 0f58b55 to 893e58c Compare January 28, 2016 20:46
@nitzanvolman
Copy link
Copy Markdown
Author

nitzanvolman commented Jan 28, 2016

@cramforce i believe this is it. please let me know if you see that i missed anything.
google bot still hates me...

@nitzanvolman
Copy link
Copy Markdown
Author

nitzanvolman commented Jan 28, 2016

I signed it!

@nitzanvolman nitzanvolman mentioned this pull request Jan 28, 2016
Closed
@cramforce
Copy link
Copy Markdown
Member

cramforce commented Jan 28, 2016

LGTM

cramforce added a commit that referenced this pull request Jan 28, 2016
@cramforce
Merge pull request #1607 from taboola/Taboola-amp-embed-suggestion
4322176 
Suggestion for an <amp-embed> builtin tag
@cramforce cramforce merged commit 4322176 into ampproject:master Jan 28, 2016
@rudygalfi rudygalfi mentioned this pull request Jan 28, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@cramforce cramforce

Labels

WG: monetization

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nitzanvolman @cramforce @googlebot @powdercloud