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

Better error handling (builds on #1191) #1197

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@heff
Member

heff commented May 9, 2014

I mistakenly rebased 1191 and screwed it up, so just created a new PR.

  • Addressed comments in #1191
  • Now clearing errors on loadstart events.
  • Added some default error messages.

@heff heff referenced this pull request May 9, 2014

Closed

Better error handling #1191

@heff heff added confirmed labels May 9, 2014

Show outdated Hide outdated src/css/video-js.less
}
.vjs-error .vjs-error-display:before {
// content: @play-icon;

This comment has been minimized.

@tomjohnson916

tomjohnson916 May 12, 2014

Contributor

It's nit picking I know, but do we want these comments here for content and font?

@tomjohnson916

tomjohnson916 May 12, 2014

Contributor

It's nit picking I know, but do we want these comments here for content and font?

This comment has been minimized.

@heff

heff May 12, 2014

Member

fixed

@heff

heff May 12, 2014

Member

fixed

vjs.ErrorDisplay.prototype.update = function(){
if (this.player().error()) {
this.contentEl_.innerHTML = this.player().error().message;

This comment has been minimized.

@tomjohnson916

tomjohnson916 May 12, 2014

Contributor

Per our side conversation, curious if this can be overridden the case of a custom errors plugin.

@tomjohnson916

tomjohnson916 May 12, 2014

Contributor

Per our side conversation, curious if this can be overridden the case of a custom errors plugin.

This comment has been minimized.

@gkatsev

gkatsev May 12, 2014

Member

Wouldn't it just be a matter of the message property on the object given to player.error?

try {
 // something that throws some error
} catch (e) {
  player.error({
    code: 42,
    message: "Error 42 happened. We still don't know the question, though."
  });
}
@gkatsev

gkatsev May 12, 2014

Member

Wouldn't it just be a matter of the message property on the object given to player.error?

try {
 // something that throws some error
} catch (e) {
  player.error({
    code: 42,
    message: "Error 42 happened. We still don't know the question, though."
  });
}

This comment has been minimized.

@heff

heff May 12, 2014

Member

Yeah. I think Tom and I got this figured out in chat.

@heff

heff May 12, 2014

Member

Yeah. I think Tom and I got this figured out in chat.

This comment has been minimized.

@gkatsev

gkatsev May 12, 2014

Member

Cool, good to have this here then for future purposes.

@gkatsev

gkatsev May 12, 2014

Member

Cool, good to have this here then for future purposes.

@tomjohnson916

This comment has been minimized.

Show comment
Hide comment
@tomjohnson916

tomjohnson916 May 12, 2014

Contributor

I've made a couple of minor notes, and reached out directly to discuss customization options on the work enclosed, but otherwise think this is good to go.

Contributor

tomjohnson916 commented May 12, 2014

I've made a couple of minor notes, and reached out directly to discuss customization options on the work enclosed, but otherwise think this is good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment