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

Dailymotion player component #2017

Merged
merged 1 commit into from
Feb 15, 2016

Conversation

zeno
Copy link
Contributor

@zeno zeno commented Feb 15, 2016

Add amp-dailymotion component to display a Dailymotion video.

Include tests, documentation and example file.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

data-sharing-enable="false"
data-ui-highlight="444444"
data-ui-logo="false"
data-ui-start_screen_info="false"
Copy link
Member

Choose a reason for hiding this comment

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

The mixed - and _ feels a bit weird.

@cramforce
Copy link
Member

This looks great!

We've currently porting the video players to show a poster during pre-rendering (amp-video done, amp-youtube in progress). We should not do this in this change, but it would be great if we could do that in the future. It would require a predictable URL for a poster image of the video.

@googlebot
Copy link

CLAs look good, thanks!

@zeno
Copy link
Contributor Author

zeno commented Feb 15, 2016

Thx for the feedback @cramforce. Just committed some changes.
Sure, showing poster during pre-rendering via a predictable URL to avoid extra request shouldn't be a problem.

@cramforce
Copy link
Member

Looks great. Two more things

  1. please add your file here https://github.com/ampproject/amphtml/blob/master/test/integration/test-example-validation.js#L55 and whitelist the error here https://github.com/ampproject/amphtml/blob/master/test/integration/test-example-validation.js#L72
  2. Squash your commits to a single commit.

I added your component to #1732. It should start validating in the next few days.

@zeno
Copy link
Contributor Author

zeno commented Feb 15, 2016

Done.

Thanks @cramforce

@cramforce
Copy link
Member

LGTM
Awesome! Will merge when travis is green.

@cramforce cramforce added the LGTM label Feb 15, 2016
cramforce added a commit that referenced this pull request Feb 15, 2016
@cramforce cramforce merged commit 4ad5e17 into ampproject:master Feb 15, 2016
@ndachez
Copy link

ndachez commented Feb 19, 2016

Hello , I tried to implement "amp- dailymotion" but I still encounter some errors. The video displays correctly... In Chrome console, I have 2 errors :

  1. The attribute 'custom-element' in tag 'amp-access extension .js script' is set to the invalid value 'amp-dailymotion'
  2. The tag 'amp-dailymotion' is disallowed.

The link : http://amp.chien.fr/actualite/cristina-cordula-m6-affirme-je-ne-suis-pas-contre-la-fourrure/#development=1

Thx.

@cramforce
Copy link
Member

This should become valid within the day.

On Fri, Feb 19, 2016 at 4:53 AM, ndachez notifications@github.com wrote:

Hello , I tried to implement "amp- dailymotion" but I still encounter some
errors. The video displays correctly... In Chrome console, I have 2 errors :

  1. The attribute 'custom-element' in tag 'amp-access extension .js
    script' is set to the invalid value 'amp-dailymotion'
  2. The tag 'amp-dailymotion' is disallowed.

The link :
http://amp.chien.fr/actualite/cristina-cordula-m6-affirme-je-ne-suis-pas-contre-la-fourrure/#development=1

Thx.


Reply to this email directly or view it on GitHub
#2017 (comment).

@ndachez
Copy link

ndachez commented Feb 22, 2016

Thx cramforce !

Indeed, I have the little message : "Powered by AMP ⚡ HTML – Version 1455904100660 / AMP validation successful."

Have a nice day :)

@okeul
Copy link

okeul commented Feb 22, 2016

Hi !

This component works well but there are several errors in the console when the video is playing :
capture d ecran 2016-02-22 a 11 22 18 am

Example :
http://output.jsbin.com/dokosa/#development=1

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.

5 participants