Brightcove implementation #139
Conversation
|
Happy to discuss about the best approach |
|
would be good to test it, as we haven't used anywhere but just one site. If Thanks. On Fri, 16 Sep 2016 at 18:27 Amir [email protected] wrote:
|
|
Two suggestions:
|
|
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]
|
There was a problem hiding this comment.
This is not ready yet.
- Fix the mandatory data fields. See the Brightcove amp spec for info.
- 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 - Run the final output through the official AMP validator
https://validator.ampproject.org/
Make sure it passes there
| } | ||
| } | ||
|
|
||
| return $brightcove_code; |
There was a problem hiding this comment.
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>"); |
There was a problem hiding this comment.
This DOM element is invalid.
data-videoidneeds to bedata-video-id- Mandatory attribute
data-accountis missing
See https://www.ampproject.org/docs/reference/components/amp-brightcove
|
The build fails. https://travis-ci.org/Lullabot/amp-library/jobs/159347578
|
No description provided.