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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jan 28, 2016
Merged

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

merged 1 commit into from
Jan 28, 2016

Conversation

nitzanvolman
Copy link
Contributor

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
* @param {Object} state Optional map to be merged into the prototype
* to override the original state with new default values
*/
export function registerElementAlias(win, aliasName, sourceName, state) {
Copy link
Member

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
Contributor Author

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
Member

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
Member

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

@cramforce
Copy link
Member

@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

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
Contributor Author

@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
Member

@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
@@ -62,7 +62,7 @@ describe('example', function() {
* @constructor {!Array<!RegExp>}
*/
const errorWhitelist = [
/INVALID_ATTR_VALUE src=\.\/viewer-integr\.js/
/.*\.\/viewer-integr\.js.*|.*amp-embed.*/
Copy link
Member

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
Member

@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
@@ -2278,6 +2278,23 @@ tags: {
supported_layouts: NODISPLAY
}
}
# <amp-embed>
Copy link
Member

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
Contributor Author

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
Contributor

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
Contributor

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
@nitzanvolman
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
Contributor Author

@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
Contributor Author

I signed it!

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

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