Better error handling #1191
Better error handling #1191
Conversation
Added sinon.js for stubs in tests. Updated grunt version to satisfy peer dependency warning.
|
Is that X icon in the screenshot the one you decided to go with? Will there be any more information in the player itself or are we leaving that up to the developer? |
|
The X was the best I could do for now. I can't find a good drop in 'no-play' icon, and I'm not sure how to update the icon font either way. I've been playing around with adding descriptions for errors. At some level we have the videojs-errors plugin that will provide more rich text error messages. But for some of the basic messages I think we should show messages in core. I'm specifically thinking of the ones where the end-user can do something to fix the issue, like upgrading their browser. For errors that need the developer to fix the issue, I wonder if the console errors are enough. Though actually it might be hard to distinguish between those two situations.
|
|
The one thing I see potentially being a problem (or confusing for viewers) is where the developer doesn't really know what they're doing and slaps something like a WMV as the source. We'd say "no compatible source was found" and link to a page about HTML5 video support, which would do nothing but confuse the end-user. They might even upgrade to a different browser only to still see the same error message, which would be awesome because they'd have a shiny new browser, but frustrating because they still can't watch the original video. Maybe this is an edge case we don't really need to worry about? Assume best faith of the folks implementing the player? It just seems like there are a non-trivial amount of issues opened based almost entirely on video formats, so it feels like a legitimate concern. TL;DR, I like the last screenshot you posted, I'm just concerned we'll end up unwittingly playing a part in confusing people. |
| @@ -630,7 +620,15 @@ vjs.Player.prototype.techGet = function(method){ | |||
| * @return {vjs.Player} self | |||
| */ | |||
| vjs.Player.prototype.play = function(){ | |||
| this.techCall('play'); | |||
| if (this.error()) { | |||
There was a problem hiding this comment.
It's kind of weird to have an empty if block.
There was a problem hiding this comment.
yeah, i was considering logging an error or warning there, but i'll fix this.
@mmcc, I agree with that. I don't know that we have access to anything more specific about why it failed. How would you phrase it so it sounds less like we know that link will address your problem? |
Now clearing errors on loadstart events. Added some default error messages.
|
Closing and moving to #1197. The comments here have been addressed. |
Squashed commit of the following: commit 81d7859 Author: Steve Heffernan <[email protected]> Date: Mon May 12 12:53:59 2014 -0700 Removed unneeded comments commit c7ad732 Author: Steve Heffernan <[email protected]> Date: Fri May 9 14:29:31 2014 -0700 Addressed comments in #1191 Now clearing errors on loadstart events. Added some default error messages. commit a742239 Author: Steve Heffernan <[email protected]> Date: Wed May 7 15:38:31 2014 -0700 Fixed the error display to hide by default commit 561c3f8 Author: Steve Heffernan <[email protected]> Date: Mon May 5 10:44:47 2014 -0700 Added support for displaying a message for the error. commit 2214207 Author: Steve Heffernan <[email protected]> Date: Fri May 2 17:18:22 2014 -0700 Updated spinner to hide on all errors commit 95d7e70 Author: Steve Heffernan <[email protected]> Date: Fri May 2 15:37:44 2014 -0700 Exported ErrorDisplay commit 11ca9cd Author: Steve Heffernan <[email protected]> Date: Fri May 2 15:35:46 2014 -0700 Updated flash tech to support new errors commit 56cbe66 Author: Steve Heffernan <[email protected]> Date: Fri May 2 13:06:49 2014 -0700 Started on better error handling and displaying in the UI when an error has occurred. commit 740014c Author: Steve Heffernan <[email protected]> Date: Wed Apr 30 16:11:33 2014 -0700 Added better global log/error/warn functions. Added sinon.js for stubs in tests. Updated grunt version to satisfy peer dependency warning.
(This is currently a work in progress but it's getting far a long so I wanted to put it up for feedback)
Summary:
For logging we'd now have
vjs.log, vjs.log.warn, and vjs.log.error, which all translate to the same console functions.For the visual error, I'm just using an X now. I'd like for it to be a crossed out play button in future, similar to iOS. I don't currently think we should show any actual text in addition to the X, and instead leave that to the videojs-errors plugin that's in progress. Firefox does show a message on the player when an error happens, but other browsers just show a crossed out play button or disabled controls, or nothing at all.
Please let me know if there's any thoughts on this approach.