De-XHRify #14
De-XHRify #14
Conversation
|
It looks like this is your first pull request. 🎉 Thank you for your contribution! One of the project maintainers will triage and assign the pull request for review. We appreciate your patience. To safeguard the health of the project, please take a moment to read our code of conduct. |
imscjs-demo/js/index.js
Outdated
| //Edge/IE implement "Generic Cue", other browsers VTTCue | ||
| var Cue = window.VTTCue || window.TextTrackCue; | ||
| const Cue = window.VTTCue || window.TextTrackCue; |
There was a problem hiding this comment.
I don't think this is still needed: VTTCue is unlikely to be supported by Edge > 79. Anyway, TextTrackCue has been supported by every engine for ages; we can simplify this. (as well as the call to the Cue() that would be a new TextTrackCue()on line 25 and 30.
There was a problem hiding this comment.
As far as I can tell the correct thing here is to use VTTCue: it's supported everywhere. TextTrackCue is an abstract class, we couldn't create it.
| //create cue per timed event and render isd | ||
| for (var i = 0; i < timeEvents.length; i++) { | ||
| for (let i = 0; i < timeEvents.length; i++) { |
There was a problem hiding this comment.
Given that we modernize this example at the same time and astimeEvents is an Array. Can you double-check if we could use for...of here (and get rid of i? (Not 100% sure it is possible)
There was a problem hiding this comment.
We use i at
imsc-examples/imscjs-demo/js/index.js
Lines 29 to 31 in 943d004
Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
| //We have to provide empty string as VTTText | ||
| var myCue = new Cue(timeEvents[i], timeEvents[i + 1], ""); | ||
| // We have to provide an empty string as the VTTText | ||
| myCue = new VTTCue(timeEvents[i], timeEvents[i + 1], ""); |
There was a problem hiding this comment.
I think the standard is TextTrackCue. (Two places)
There was a problem hiding this comment.
I know basically nothing about this API, but it looks to me like you can't construct a TextTrackCue, it doesn't have a constructor. From https://developer.mozilla.org/en-US/docs/web/api/texttrackcue:
TextTrackCue is an abstract class which is used as the basis for the various derived cue types, such as VTTCue; you will instead work with those derived types.
There was a problem hiding this comment.
I have tested this demo in Firefox and Chrome with VTTCue and it works, and with TextTrackCue, and it doesn't.
Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
|
Just added a small suggestion clarifying terms |
Co-authored-by: Pierre-Anthony Lemieux <pal@sandflow.com>
There was a problem hiding this comment.
Approving this based on other reviews to check if I have merge rights. As @teoli2003 has indicated, this PR is ready to be merged.
|
Congratulations on your first merged pull request. 🎉 Thank you for your contribution! Did you know we have a project board with high-impact contribution opportunities? We look forward to your next contribution. |
Part of openwebdocs/project#156. I had to keep the event listeners as non-arrow functions, I think because they depend on the
thisvalue.