Brightcove implementation

[alex-moreno]

No description provided.

[alex-moreno]

Happy to discuss about the best approach

[alex-moreno]

would be good to test it, as we haven't used anywhere but just one site. If
you can checkout that branch and check that all works as expected, that
would be great too.

Thanks.

On Fri, 16 Sep 2016 at 18:27 Amir notifications@github.com wrote:

I need support for amp-brightcove as well

If the unit test is the only thing holding up this pr, I'll be happy to
help fix it

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#139 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB-5zhePgENkGGqOEA39Qo4mqeV-97Hyks5qqtGHgaJpZM4J6uR3
.

[sebastianbenz]

Two suggestions:

• Please fix the failing tests seems like you need to import IframeBrighCoveTagTransformPass in AMP.php
• Please add a corresponding test case: create a new test case (an .html file) that captures the code change/fix you made. Run amp-regen to create the corresponding expected output file for your new test case. Now the test suite has a new test with expected output. Commit the .html and .html.out file into git.

[alex-moreno]

Hi Sebastian,

I'll have it fixed it during the week, thanks for your time reviewing this.

On Mon, 19 Sep 2016 at 20:03 Sebastian Benz notifications@github.com
wrote:

Two suggestions:

• Please fix the failing tests seems like you need to import
  IframeBrighCoveTagTransformPass in AMP.php
• Please add a corresponding test case: create a new test case (an
  .html file) that captures the code change/fix you made. Run amp-regen to
  create the corresponding expected output file for your new test case. Now
  the test suite has a new test with expected output. Commit the .html and
  .html.out file into git.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#139 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB-5zmA6uHKpSqryXi6vkWPAM4Vlzh8uks5qrtyHgaJpZM4J6uR3
.

[SGudbrandsson]

This is not ready yet.

1. Fix the mandatory data fields. See the Brightcove amp spec for info.
2. Create a test cases for multiple variations of the Brightcove code.
   2.1 Use examples from the real world (search for brightcove players online)
   2.2 Use all available implementations of the brightcove player
   https://support.brightcove.com/en/video-cloud/docs/publishing-brightcove-player
   https://support.brightcove.com/en/video-cloud/docs/embedding-video-cloud-players-iframes
3. Run the final output through the official AMP validator
   https://validator.ampproject.org/
   Make sure it passes there

[SGudbrandsson]

You need to make sure to return valid HTML code here..
wrap the return value in htmlspecialchars

See example here:
https://github.com/Lullabot/amp-library/blob/master/src/Pass/IframeYouTubeTagTransformPass.php#L134

[SGudbrandsson]

This DOM element is invalid.

• data-videoid needs to be data-video-id
• Mandatory attribute data-account is missing

See https://www.ampproject.org/docs/reference/components/amp-brightcove

[SGudbrandsson]

The build fails. https://travis-ci.org/Lullabot/amp-library/jobs/159347578

PHP Fatal error: Class 'Lullabot\AMP\Pass\IframeBrighCoveTagTransformPass' not found in /home/travis/build/Lullabot/amp-library/src/AMP.php on line 439