Extend createTimeRange to enable creation of TimeRanges with multiple ranges #2604
Conversation
imbcmdth
force-pushed
the
multiple-timeranges
branch
from
7c3e1e0
to
46117a1
Compare
| }; | ||
| } | ||
|
|
||
| function getRange(fnName, valueIndex, ranges, rangeIndex){ | ||
| if (rangeIndex === undefined) { |
There was a problem hiding this comment.
No args here actually throws an error with native timeranges. If we want them to be interchangeable we probably shouldn't let people do this.
There was a problem hiding this comment.
Just saw the note about backward compatibility. Now I'm on the fence.
There was a problem hiding this comment.
We could add a deprecation warning to the no-index case.
There was a problem hiding this comment.
That would be ok with me. Do we actually use it in that way anywhere? If we are then either way we'd have to update those uses.
There was a problem hiding this comment.
This one's still unresolved. Are we punting on it? @imbcmdth
There was a problem hiding this comment.
@heff I added the deprecation warning, none of our plugins use the no-index style but we can't be sure that no one else has. I'd rather we provide a clear indication that we intend to remove that "feature" in the near future.
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 46117a1 (Please note that this is a fully automated comment.) |
|
Looks good so far. We'll need to update the video.js file and export both naming versions on the videojs object also. And now that I see it here we could probably just do the aliasing in the video.js file, instead of in the time-ranges.js file, but up to you. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 7c3e1e0a8a18029424f3f54868967e406691fe93 (Please note that this is a fully automated comment.) |
|
That would require changes everywhere |
Exporting it on the videojs object would require that? I just mean having both videojs.createTimeRange and videojs.createTimeRanges |
|
I meant that:
Would require changes everywhere that createTimeRange is used via an import statement. |
|
Ahh, I see. Ok. Sent from mobile |
…on to the videojs object
|
@heff Added the new |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: d7b7baa (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: eefb775 (Please note that this is a fully automated comment.) |
|
lgtm! |
No checks are performed to ensure that the resulting TimeRanges-like object is "normalized" as the spec requires.
Supports not passing an argument to
startandendand returns the first range for backward compatibility.Throws an error with Chrome's error message if
startorendis passed an index that is outside the supported ranges.