Add amp-izlesene extension to embed videos from izlesene.com by merty · Pull Request #6126 · ampproject/amphtml

merty · 2016-11-10T13:07:33Z

We're the leading Turkish video network with millions of video views every single day. To enable our users embed videos on their AMP pages as well, we've created the amp-izlesene extension.

Contributors: @gberkman @ferrerodbgm

jridgewell · 2016-11-10T20:14:25Z

/to @aghassemi

aghassemi · 2016-11-10T21:15:48Z

@merty Thanks for the PR. I will get back to you with the code review soon, meanwhile, I have a question for you: Does the izlesene's iframe embed have a postMessage API that would allow to send commands such as pause, play, mute, unmute and receive events such as played, paused? We are requesting all new video players to implement our video-interface which would require such functionality.

aghassemi · 2016-11-11T17:30:04Z

examples/izlesene.amp.html

+<h2>Izlesene</h2>
+
+
+<amp-izlesene


Remove this one, just keep the layout responsive example.

aghassemi · 2016-11-11T17:30:34Z

extensions/README.md

 | [`amp-dailymotion`](amp-dailymotion/amp-dailymotion.md) | Displays a [Dailymotion](https://www.dailymotion.com) video. |
 | [`amp-gfycat`](amp-gfycat/amp-gfycat.md) | Displays a [Gfycat](https://gfycat.com) video GIF. |
 | [`amp-jwplayer`](amp-jwplayer/amp-jwplayer.md) | Displays a cloud-hosted [JW Player](https://www.jwplayer.com/). |
+| [`amp-izlesene`](amp-izlesene/amp-izlesene.md) | Displays a Izlesene video. |


move to before jwplayer for correct alphabetical sorting.

aghassemi · 2016-11-11T17:30:41Z

extensions/amp-izlesene/0.1/amp-izlesene.js

@@ -0,0 +1,64 @@
+/**
+ * Copyright 2015 The AMP HTML Authors. All Rights Reserved.


aghassemi · 2016-11-11T17:31:23Z

extensions/amp-izlesene/0.1/amp-izlesene.js

+ */
+
+ import {getDataParamsFromAttributes} from '../../../src/dom';
+ import {addParamsToUrl} from '../../../src/url';


Sort by import path.

aghassemi · 2016-11-11T17:35:43Z

extensions/amp-izlesene/0.1/amp-izlesene.js

+
+  /** @override */
+  preconnectCallback(onLayout) {
+    this.preconnect.url('https://www.izlesene.com', onLayout);


also preload the exact Url the will become the iframe source. (see comment below about creating a getVideoIframeSrc_ so you can do this.preconnect.preload(this.getVideoIframeSrc_());)

aghassemi · 2016-11-11T17:49:56Z

extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii

+# limitations under the license.
+#
+
+tags: {  # amp-izlesene


@honeybadgerdontcare for validator review.

Validation looks good, just need to update the two spec_url locations. Thanks!

aghassemi · 2016-11-11T17:50:30Z

extensions/amp-izlesene/amp-izlesene.md

@@ -0,0 +1,73 @@
+<!---
+Copyright 2015 The AMP HTML Authors. All Rights Reserved.


aghassemi · 2016-11-11T17:52:08Z

extensions/amp-izlesene/amp-izlesene.md

+  </tr>
+  <tr>
+    <td width="40%"><strong>Examples</strong></td>
+    <td><a href="https://ampbyexample.com/components/amp-izlesene/">Annotated code example for amp-izlesene</a></td>


This example does not exist, please remove for you and add back after you have submitted a PR to https://github.com/ampproject/amp-by-example to add an example.

aghassemi · 2016-11-11T17:56:00Z

extensions/amp-izlesene/amp-izlesene.md

+
+- `data-param-showcontrols=1` becomes `&showcontrols=1`
+
+Because of limitations in mobile browsers, the `autoplay` param is currently not supported. Follow [this issue](https://github.com/ampproject/amphtml/issues/3799) for updates on autoplay support in AMP.


This is no longer true and AMP manages autoplay if your player can implement the "video-interface". Depending on the outcome of that discussion, we need to update this paragraph.

aghassemi · 2016-11-11T17:56:48Z

gulpfile.js

 declareExtension('amp-image-lightbox', '0.1', true);
 declareExtension('amp-instagram', '0.1', false);
 declareExtension('amp-install-serviceworker', '0.1', false);
+declareExtension('amp-izlesene', '0.1', false, 'NO_TYPE_CHECK');


remove NO_TYPE_CHECK and run gulp check-types to check types and fix issues accordingly.

honeybadgerdontcare · 2016-11-11T18:16:04Z

extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii

+      error_message: "contents"
+    }
+  }
+  spec_url: "https://www.ampproject.org/docs/reference/extended/amp-izlesene.html"


Since this isn't on ampproject.org yet, lets point the spec_url to where the markup that's in this PR will be:
https://github.com/ampproject/amphtml/blob/master/extensions/amp-izlesene/amp-izlesene.md

honeybadgerdontcare · 2016-11-11T18:17:03Z

extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii

+    value_regex: "[0-9]+"
+  }
+  attr_lists: "extended-amp-global"
+  spec_url: "https://www.ampproject.org/docs/reference/extended/amp-izlesene.html"


Also here:
Since this isn't on ampproject.org yet, lets point the spec_url to where the markup that's in this PR will be:
https://github.com/ampproject/amphtml/blob/master/extensions/amp-izlesene/amp-izlesene.md

Refering the Markdown document instead led to an "Expected/Saw" error so reverted this change to refer to the HTML document.

Can you include what that error was? It shouldn't happen and should be able to link to the markdown in the PR. If you're still having issues better to remove the spec_url entirely as we don't want URLs in error messages to 404 and we put spec_urls in error messages.

Sure, you can see it here.

honeybadgerdontcare · 2016-11-14T21:06:27Z

extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii

+  html_format: AMP4ADS
+  tag_name: "SCRIPT"
+  spec_name: "amp-izlesene extension .js script"
+  mandatory_parent: "HEAD"


after mandatory_parent: "HEAD", please add: unique: true. This prevents multiple inclusions of the same script file.

aghassemi · 2016-11-15T17:36:06Z

extensions/amp-izlesene/0.1/test/test-amp-izlesene.js

+      if (opt_responsive) {
+        izlesene.setAttribute('layout', 'responsive');
+      }
+      iframe.doc.body.appendChild(izlesene);


replace these three lines with
return iframe.addElement(izlesene);

aghassemi · 2016-11-21T18:41:54Z

@merty when you are ready, please make the requested changes and let us know when done to review again. Thanks!

aghassemi · 2016-12-14T21:39:09Z

@merty Just a friendly ping to make the requested changes before this PR can be merged. Thanks!

aghassemi · 2017-01-03T19:44:14Z

@gberkman Please let me know when this is ready for review again. Thanks!

merty · 2017-02-13T09:04:31Z

@aghassemi It should now be ready for review. Could you please help us fix the following two issues?

honeybadgerdontcare · 2017-02-13T17:36:19Z

extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii

+
+tags: {  # amp-izlesene
+  html_format: AMP
+  html_format: AMP4ADS


This is the second error. This extension is not whitelisted for AMP4ADS so it shouldn't be listed for it.

Removed 👍

honeybadgerdontcare · 2017-02-13T17:38:00Z

extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii

+  tag_name: "SCRIPT"
+  spec_name: "amp-izlesene extension .js script"
+  mandatory_parent: "HEAD"
+  unique: true


This is the first error. After unique: true add another line with also_requires_tag: amp-izlesene.

Actually, we're moving this to using satisfies and requires instead of also_requires_tag. So instead of putting also_requires_tag: amp-izlesene please put above under spec_name, satisfies: "amp-izlesene extension .js script" and below in tag_name: "AMP-IZLESENE" replace the also_requires_tag with requires: "amp-izlesene extension .js script". See this PR as an example.

Still get this one, but now at a later stage:

honeybadgerdontcare · 2017-02-13T17:45:52Z

@merty I marked in the protoascii how to address those two errors and filed an issue to make the errors more clear #7509

jridgewell · 2017-02-13T21:51:51Z

Ping @aghassemi.

honeybadgerdontcare · 2017-02-13T22:45:32Z

extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii

+}
+tags: {  # <amp-izlesene>
+  html_format: AMP
+  html_format: AMP4ADS


Please remove html_format: AMP4ADS

honeybadgerdontcare · 2017-02-13T22:46:23Z

Note there are three issues outstanding with the protoascii.

aghassemi · 2017-02-13T23:37:20Z

extensions/amp-izlesene/0.1/amp-izlesene.js

+   * @override
+   */
+  preconnectCallback(opt_onLayout) {
+    this.preconnect.preload(this.getVideoIframeSrc_());


preload is too heavy. Please just do this.preconnect.url(this.getVideoIframeSrc_()); to preconnect.

honeybadgerdontcare · 2017-02-14T16:11:04Z

Please reference the markdown in this PR. We don't want error messages with URLs that 404.

…

On Mon, Feb 13, 2017 at 11:22 PM, Mert Yazıcıoğlu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii <#6126>: > + } + spec_url: "https://www.ampproject.org/docs/reference/extended/amp-izlesene.html" +} +tags: { # <amp-izlesene> + html_format: AMP + html_format: AMP4ADS + tag_name: "AMP-IZLESENE" + disallowed_ancestor: "AMP-SIDEBAR" + also_requires_tag: "amp-izlesene extension .js script" + attrs: { + name: "data-videoid" + mandatory: true + value_regex: "[0-9]+" + } + attr_lists: "extended-amp-global" + spec_url: "https://www.ampproject.org/docs/reference/extended/amp-izlesene.html" Refering the Markdown document instead led to an "Expected/Saw" error so reverted this change to refer to the HTML document. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6126>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABR0lZy9q7fOz__dUwrLW512h1dwvIOPks5rcVZGgaJpZM4Kuo_U> .

honeybadgerdontcare · 2017-02-14T18:21:54Z

extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii

+  spec_name: "amp-izlesene extension .js script"
+  satisfies: "amp-izlesene extension .js script"
+  mandatory_parent: "HEAD"
+  unique: true


Add extension_unused_unless_tag_present: "amp-izlesene" after unique: true to address the last travis error.

honeybadgerdontcare · 2017-02-14T19:02:49Z

Ah, that's because your .out file references the ampproject.org url. You can update that with where your markdown will be in master. https://github.com/nokta/amphtml/blob/ad305fff22ccef80bc0c86a3fae9215639dab522/extensions/amp-izlesene/0.1/test/validator-amp-izlesene.out

…

On Tue, Feb 14, 2017 at 10:59 AM, Mert Yazıcıoğlu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In extensions/amp-izlesene/0.1/validator-amp-izlesene.protoascii <#6126>: > + } + spec_url: "https://www.ampproject.org/docs/reference/extended/amp-izlesene.html" +} +tags: { # <amp-izlesene> + html_format: AMP + html_format: AMP4ADS + tag_name: "AMP-IZLESENE" + disallowed_ancestor: "AMP-SIDEBAR" + also_requires_tag: "amp-izlesene extension .js script" + attrs: { + name: "data-videoid" + mandatory: true + value_regex: "[0-9]+" + } + attr_lists: "extended-amp-global" + spec_url: "https://www.ampproject.org/docs/reference/extended/amp-izlesene.html" Sure, you can see it here <https://travis-ci.org/ampproject/amphtml/builds/201019165>. <https://camo.githubusercontent.com/4741cb0223d83ef960e33627392fd5a2c4464503/687474703a2f2f692e6d6572742e70772f56754b382b> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6126>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABR0lQQLwuaIs9eyG6IM9f6kgzU_smsJks5rcfmRgaJpZM4Kuo_U> .

merty · 2017-02-14T20:22:19Z

@honeybadgerdontcare Changing both the .out file and the protoascii file to refer to the same URL did the trick, thanks!

Build looks good to go now 👍

CC/ @aghassemi

honeybadgerdontcare · 2017-02-14T20:40:00Z

Great!
@aghassemi validation looks good.

aghassemi · 2017-02-15T00:06:40Z

Looks good @merty! Thanks for contributing. Merged.

ghost · 2017-02-21T16:07:29Z

Has the validator.js been updated for this component? I ask because I've tried using this component and I receive the following error in the console: The tag 'amp-izlesene' is disallowed. I'm discovering that the script isn't on the CDN --https://cdn.ampproject.org/v0/amp-izlesene-0.1.js.

aghassemi · 2017-02-21T16:37:57Z

@bpaduch This component is not in prod yet (will be at the end of this week) but it is in the Dev Channel right now.

honeybadgerdontcare · 2017-02-21T18:15:13Z

@bpaduch Validation changes can take 1-2 weeks to get released to prod.

ghost · 2017-02-21T18:25:51Z

Okay, if it's in the Dev Channel, wouldn't that categorize the component as "experimental" rather than "stable"?

aghassemi · 2017-02-21T19:25:42Z

"experimental" normally means there is an experiment flag around the component and it may stay experimental for a while. In this case, there is no experiment. What we should have done is not pushing the documentation until this made it to prod. Or maybe we can introduce a new "Coming Soon" category where we push the documentation early but telling the user it may take couple of weeks before they can use it. "doc being out of sync with code" happens enough times that I think it is totally worth figuring out a solution for it.

…ect#6126)

jridgewell assigned aghassemi Nov 10, 2016

jridgewell added 3P labels Nov 10, 2016

aghassemi suggested changes Nov 11, 2016

View reviewed changes

honeybadgerdontcare reviewed Nov 11, 2016

View reviewed changes

honeybadgerdontcare reviewed Nov 14, 2016

View reviewed changes

aghassemi reviewed Nov 15, 2016

View reviewed changes

honeybadgerdontcare reviewed Feb 13, 2017

View reviewed changes

honeybadgerdontcare mentioned this pull request Feb 13, 2017

ValidatorRulesMakeSense errors should be more verbose #7509

Closed

honeybadgerdontcare reviewed Feb 13, 2017

View reviewed changes

adelinamart removed the To be deprecated - 3P label Feb 13, 2017

aghassemi suggested changes Feb 13, 2017

View reviewed changes

honeybadgerdontcare reviewed Feb 14, 2017

View reviewed changes

Add amp-izlesene extension to embed videos from izlesene.com

0ff07eb

aghassemi approved these changes Feb 15, 2017

View reviewed changes

aghassemi merged commit f86437a into ampproject:master Feb 15, 2017

ghost mentioned this pull request Feb 21, 2017

Update component availability + fix style and wording #7696

Merged

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Add amp-izlesene extension to embed videos from izlesene.com (ampproj…

805a6f0

…ect#6126)

		@@ -0,0 +1,64 @@
		/**
		* Copyright 2015 The AMP HTML Authors. All Rights Reserved.

		@@ -0,0 +1,73 @@
		<!---
		Copyright 2015 The AMP HTML Authors. All Rights Reserved.


		- `data-param-showcontrols=1` becomes `&showcontrols=1`

		Because of limitations in mobile browsers, the `autoplay` param is currently not supported. Follow [this issue](https://github.com/ampproject/amphtml/issues/3799) for updates on autoplay support in AMP.

		<h2>Izlesene</h2>


		<amp-izlesene

Conversation

merty commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell commented Nov 10, 2016

Uh oh!

aghassemi commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi commented Nov 21, 2016

Uh oh!

aghassemi commented Dec 14, 2016

Uh oh!

aghassemi commented Jan 3, 2017

Uh oh!

merty commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare commented Feb 13, 2017

Uh oh!

jridgewell commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare commented Feb 14, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merty commented Nov 10, 2016 •

edited

Loading