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

export Component.prototype.fadeIn/Out #581

Closed
wants to merge 1 commit into from

Conversation

sansb
Copy link
Contributor

@sansb sansb commented Jun 18, 2013

Requesting that fadeIn and fadeOut are exported.

@heff
Copy link
Member

heff commented Jun 24, 2013

Looks good. Pulling in. There's also a new set of API tests coming in that you could piggy back on to add a test for this.
https://github.com/videojs/video.js/pull/588/files#L3R21

@heff heff closed this in 0420760 Jun 24, 2013
@sansb
Copy link
Contributor Author

sansb commented Jun 25, 2013

Cool, thanks. I meant to leave a longer comment on this but got side-tracked: What is the guideline for which methods get exported? I also found a need for the addClass/removeClass methods and have them exported in a branch. It's not that I can't write my own addClass/removeClass but I thought since they're already written in the library it would make sense to use them.

And yes I'll add some tests for fadeIn/fadeOut.

@heff
Copy link
Member

heff commented Jun 27, 2013

The guideline is basically just ask. Or even better, pull request. We mangled everything we didn't need by default to make the player as lean as possible, and we're exposing things that have valid use cases as they come in.

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.

2 participants