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

Added appendNotSupportedMessage function. #758

Closed
wants to merge 2 commits into from

Conversation

ajbogh
Copy link

@ajbogh ajbogh commented Sep 30, 2013

Can be called from parent website in case of external errors.

Website can change the notSupportedMessage then call this function to display a custom error. Messages will reuse existing error containers in case someone tries to throw an error message and the video throws a different error. A class of vjs-error is used for styling purposes. An id of vjs-error is used for selection purposes.

@gkatsev
Copy link
Member

gkatsev commented Sep 30, 2013

@ajbogh
Copy link
Author

ajbogh commented Oct 1, 2013

This PR attempts to functionalize the code in a way that extensions, such as videojs-errors, can use. It won't break the extensions, but it also gives developers the opportunity to avoid implementing extensions where they might not be appropriate or necessary.

@gkatsev
Copy link
Member

gkatsev commented Oct 1, 2013

I think the idea is to keep what is in core really small and all other functionality be done in plugins. I'm not sure the functionality provided here is warrants being added directly to core rather than via a plugin.

@ajbogh
Copy link
Author

ajbogh commented Oct 1, 2013

If so, should we not remove the notSupportedMessage from core as well? This message seems really specific to the error functionality for one error type and is not generic enough to warrant a place in core. It also doesn't provide a suitable means to select the error message for stylistic purposes. This PR hopes to rectify that problem, however it does leave specific error code in core where it, arguably, shouldn't belong.

@heff
Copy link
Member

heff commented Oct 1, 2013

@ajbogh, thanks for the contribution. I'm not totally clear on the core needs this addresses. Can we list them out?

  • Should allow for an error to be displayed to the user
  • Should allow for a custom error to be triggered (?)
    Is that right, and what else?

Currently we have the videojs-errors plugin and the 'notSupported' option for customizing the message. Where do those fall short?

@ajbogh
Copy link
Author

ajbogh commented Oct 2, 2013

  • Allows for selecting of the error message for styling purposes (new CSS class on paragraph).
  • Functionalizes message creation code for unit testing and readability.
  • Allows website to dynamically update the error message based on some new information (language choice, etc.)

The notSupportedMessage can only be set once, it's can't be dynamically updated after the player element has been created. This code attempts to fix that problem and it sets up a precedent that should be followed -- all optional items (in this case an error message) should be able to be dynamically modified after initialization. If this precedent is followed then websites that support dynamic language switching could change the error message on the fly without reloading the page.

@gkatsev
Copy link
Member

gkatsev commented Oct 2, 2013

I believe that the notSupportedMessage can be changed manually either when creating a new videojs player or after the fact by getting the options and setting the notSupportedMessage property on that object.

@ajbogh
Copy link
Author

ajbogh commented Oct 2, 2013

Given the the notSupportedMessage is only written when the player.src function is called (line 752 - player.js), you would have to re-set the sources in order to change the message. This is of course assuming you were able to change the option before doing so. Even then, the player class appends the paragraph tag without regard to existing objects such that you would end up with 2 paragraphs -- one that contained the first error message and another that contained the updated error. In the current code there's no way to identify if a notSupportedMessage had been written to avoid re-writing a new message.

This is what this PR is trying to avoid.

It would be nice to get this merged in because it would help my work at Brightcove out. Currently I'm performing additional selections using JQuery to update the message because Backbone.js calls the render function twice where the first time the function is called with a blank source array, leading to an error message being created. The second time I may change the error message depending on whether it was an API error or a source error. To do this I use JQuery to select the paragraph element and replace the content. I would prefer if video.js had the functionality to change its own message so that I don't need the extra code in the Analytics Dashboard.

@heff
Copy link
Member

heff commented Mar 4, 2014

@gkatsev, any additional thoughts on this one?

@heff
Copy link
Member

heff commented Apr 25, 2014

Thanks for this @ajbogh. I'm going to try to address this with #869.

@heff heff closed this Apr 25, 2014
@heff
Copy link
Member

heff commented May 13, 2014

You can now call player.error(0, 'My custom error'); and it will trigger an error event, display the message, and add a vjs-error classname to the element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs: more info Please make enough detailed information is added to be actionable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants