-
Notifications
You must be signed in to change notification settings - Fork 23
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
FLUID-4558: Error handling #92
base: main
Are you sure you want to change the base?
Conversation
… to identify when/where errors happen
…an amara caption.
…erly when there are load errors.
css/VideoPlayer.css
Outdated
@@ -374,6 +488,13 @@ a.fl-videoPlayer-button-wrapper { | |||
.fl-videoPlayer-languageMenu .fl-videoPlayer-menuItem:hover { | |||
background-color: #ffcc00; | |||
} | |||
.fl-videoPlayer-languageMenu .fl-videoPlayer-menuItem-disabled:after { | |||
/* TODO: This is not internationalizable */ | |||
content: " (unavailable)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you are doing this here if it's not internationalizable?
Conflicts: js/VideoPlayer_transcript.js
js/ErrorPanel.js
Outdated
retryButton: ".flc-errorPanel-retryButton", | ||
retryButtonText: ".flc-errorPanel-retryButton-text" | ||
}, | ||
retryCallback: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to expose a callback here instead of having an event that we can attach listeners to?
I've updated this branch to use an event for the retry callback, but I haven't yet addressed the other comments. Regarding event on track load error, in Safari it seems the track element (not its TextTrack object) does fire an "error" event. We should use that when it's available. The Captionator polyfill does NOT fire this error. |
* Arrays will work here as well. | ||
*/ | ||
that.show = function (values) { | ||
that.locate("message").text(fluid.stringTemplate(that.options.strings.messageTemplate, fluid.makeArray(values))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps instead of converting these to arrays and using %0 and %1 in the string template. We could switch to using %language and %medium in the template and change the values to being an object. This would probably make it easier to read the template and ensure that the correct data is put in each token.
@acheetham I ran through the unit tests. There seems to be some strange timing issues happening that are causing the tests to fail when the all-tests.html file is run. There is also an issue with the VideoPlayerIntegration-tests for "Video Player Error Handling Tests: Transcript (amara) load error", which works when run by itself but not as part of the test suite. Other than that the unit tests seem fine in Firefox. Webkit browsers seem to have more of these timing issues. |
|
No description provided.