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

Re-expose TextTrack #2625

Closed
wants to merge 2 commits into from

Conversation

misteroneill
Copy link
Member

Fixes #2623

@pam
Copy link

pam commented Sep 22, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: c7dd29bc6de68228dec4098b490fe4df7c1f234b
Build details: https://travis-ci.org/pam/video.js/builds/81602820

(Please note that this is a fully automated comment.)

@gkatsev
Copy link
Member

gkatsev commented Sep 22, 2015

LGTM. Though, still feels a bit weird to register it as a component.
Also, do we want to expose the other text track definitions as well? I guess those can wait till 5.1 if needed.

@heff
Copy link
Member

heff commented Sep 22, 2015

Yeah, components are a pretty specific concept in video.js with expected functions and such, so I don't think it's safe to register TextTrack as one, at least as it is. And maybe more importantly we don't want to set a trend of registering non-components this way.

I think if we want this done quickly we should just export TextTrack onto the videojs object.

@misteroneill
Copy link
Member Author

I agree that it's weird as a component. Would it be preferable to simply expose it as videojs.TextTrack? Ultimately, it might make sense to have a getter/setter for non-component classes in the future.

@dmlap
Copy link
Member

dmlap commented Sep 22, 2015

+1 to videojs.TextTrack. I don't think we should add getters/setters for this sort of thing. It's just a class, we don't need a dynamic lookup mechanism for it.

@misteroneill
Copy link
Member Author

I thought it might make sense for consistency with getComponent/registerComponent, but that's neither here nor there.

I'll make the change to expose it as a property of videojs instead.

@heff
Copy link
Member

heff commented Sep 22, 2015

lgtm

@heff heff closed this in 3f8be86 Sep 22, 2015
@heff heff mentioned this pull request Sep 22, 2015
@pam
Copy link

pam commented Sep 22, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: e220c10
Build details: https://travis-ci.org/pam/video.js/builds/81609899

(Please note that this is a fully automated comment.)

@misteroneill misteroneill deleted the re-expose-texttrack branch September 22, 2015 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants