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

5.0: consider exposing some dom.js methods #2683

Closed
nickygerritsen opened this issue Oct 7, 2015 · 8 comments
Closed

5.0: consider exposing some dom.js methods #2683

nickygerritsen opened this issue Oct 7, 2015 · 8 comments

Comments

@nickygerritsen
Copy link
Contributor

We are currently using some methods from dom.js in our project.
We can not directly import that file from outside of the video.js package, as it seems this is not allowed by browserify / babelify.

This means we now have a copy of some of these functions, which is a shame.

It would be better if either these functions are exposed separately or if the whole dom file is exposed.

We currently use these methods:

  • createEl
  • hasElClass
  • addElClass
  • removeElClass

Is this something that you would consider changing?

@nickygerritsen
Copy link
Contributor Author

Btw I could create a PR for this :).

Furthermore: we need a check on IE9 in our code (because the IMA SDK does not work in IE9). Could such a check be added to browser.js? I could also PR that.

@nickygerritsen
Copy link
Contributor Author

Any thoughts on this?

@gkatsev
Copy link
Member

gkatsev commented Oct 13, 2015

I don't think we're against exposing those if they're useful to users.
The question is whether we should expose all the dom functions under videojs.dom or directly on the videojs object. I'm leaning towards the videojs object because the methods have rather unique names and in previous discussions we wanted to do that so we don't just become a utility library.

@nickygerritsen
Copy link
Contributor Author

So should I then create a PR with the four specifc methods?

And what about the IE9 check?

@gkatsev
Copy link
Member

gkatsev commented Oct 19, 2015

I think we've previously been reluctant to add user agent checks that videojs itself doesn't use.

@nickygerritsen
Copy link
Contributor Author

Ok if al good I'll then create a PR with the four mentioned methods, ok?

@gkatsev
Copy link
Member

gkatsev commented Oct 19, 2015

That would be awesome. Thanks!

@gkatsev
Copy link
Member

gkatsev commented Nov 9, 2015

#2754 has been merged which should address this.

@gkatsev gkatsev closed this as completed Nov 9, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants