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

fix critical js error in some case #26

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix critical js error in some case #26

wants to merge 4 commits into from

Conversation

Bouki
Copy link

@Bouki Bouki commented May 30, 2016

there is 1 improvement (poster is now correctly loaded using ajax with jquery)
and 1 bug fix, in some case loadApi() is called but the iframe still doesn't exists yet, this cause a fatal error (from dailymotion js) and avoid any further js to work on the page

@@ -66,7 +66,7 @@ var Dailymotion = (function (_Tech) {

if (typeof this.videoId !== 'undefined') {
this.setTimeout(function () {
_this.setPoster('//api.dailymotion.com/video/' + _this.videoId + '?fields=poster_url&ads=false');
_this.setPoster('https://api.dailymotion.com/video/' + _this.videoId + '?fields=thumbnail_large_url');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'll be better to stick to //api. rather than enforcing https://, that way it'll ensure compatibility from all sort of requests. Don't you think so?

@Bouki
Copy link
Author

Bouki commented Jul 19, 2016

the api doesn't work with http protocol, only https
also https requests are always allowed unlinke http

this.poster_ = poster;
this.trigger('posterchange');
var baseClass = this;
$.getJSON(poster, function(data) {
Copy link
Owner

@benjipott benjipott Jul 19, 2016

Choose a reason for hiding this comment

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

Use Videojs get instead if jquey
videojs.xhr({

  •   body: someJSONString,
    
  •   uri: "/foo",
    
  •   headers: {
    
  •     "Content-Type": "application/json"
    
  •   }
    
  • }, function (err, resp, body) {
    
  •   // check resp.statusCode
    
  • });
    

https://github.com/videojs/video.js/blob/b7fdf21a05fca4158041b95b30f3950495414444/src/js/video.js#L516

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