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.

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

Brightcove implementation #139

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alex-moreno
Copy link

No description provided.

@alex-moreno
Copy link
Author

Happy to discuss about the best approach

@alex-moreno
Copy link
Author

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 [email protected] 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
Copy link
Contributor

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
Copy link
Author

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 [email protected]
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
.

Copy link
Contributor

@SGudbrandsson SGudbrandsson left a comment

Choose a reason for hiding this comment

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

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

}
}

return $brightcove_code;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

/** @var \DOMElement $new_dom_el */
$el->after("<amp-brightcove data-videoid=\"$brightcove_code\" layout=\"responsive\"></amp-brightcove>");
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants