-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
amp-brightcove - pass custom params and allow data-player
#1909
amp-brightcove - pass custom params and allow data-player
#1909
Conversation
/cc @Gregable @powdercloud on any possible validator needs. |
LGTM. Test errors seem unrelated. This should not affect validation. Please squash the commits and this is good to go. |
Allow `data-player` as intented as well as `data-playerid` Use `data-player` in docs Document `data-param-*`
3b045ea
to
c3a8f98
Compare
Squashed. Thanks. |
amp-brightcove - pass custom params and allow `data-player`
Reverted in #1924 due to (seemingly unrelated) test failures. I'll be investigating today. |
It's beyond me why they're failing. Sorry for causing trouble. |
Maybe https://github.com/ampproject/amphtml/blob/master/validator/validator.protoascii#L2212 should be changed to check for |
Revert "Revert "Merge pull request #1909 from BrightcoveOS/amp-brightcove-params""
Following up on this thread to make sure we're validating amp-brightcove correctly. The current rules are: In order to help publishers find bugs, It looks like we may also want to:
Thoughts? |
A few updates to amp-brightcove
data-param-*
attributes will now be passed camel cased in the query string of the player iframe src, in line with Append data-param-* attributes to amp-iframe src as query params. #1537data-param-*
data-param-*
amp-brightcove
usesdata-player-id
as an attribute whereas the standard non-AMP embed for the Brightcove Player usesdata-player
.data-player
had been intended, hence the confusion in the docs (Brightcove player attribute #1793). Since publishers may well assume the same attribute as the non-AMP embed,amp-brightcove
now supports either.data-player
but note the compatibility withdata-player-id