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

feat: Add videojs.hookOnce method to allow single-run hooks. #4672

Merged
merged 1 commit into from Oct 30, 2017

Conversation

Projects
None yet
3 participants
@misteroneill
Member

misteroneill commented Oct 18, 2017

Description

This adds a videojs.hookOnce() method, which allows single-run hooks.

Also, cleaned up the guide for readability and grammar/spelling/typos.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
  • Reviewed by Two Core Contributors
@gkatsev

Mostly good. One concern.

@@ -140,8 +140,8 @@ videojs.hooks_ = {};
* @param {string} type
* the lifecyle to get hooks from
*
* @param {Function} [fn]
* Optionally add a hook to the lifecycle that your are getting.
* @param {Function|Function[]} [fn]

This comment has been minimized.

@gkatsev

gkatsev Oct 19, 2017

Member

I don't think we should document or officially support multiple handlers in a single hooks call. It just encourages bad code.

@gkatsev

gkatsev Oct 19, 2017

Member

I don't think we should document or officially support multiple handlers in a single hooks call. It just encourages bad code.

This comment has been minimized.

@misteroneill

misteroneill Oct 19, 2017

Member

It's already documented for the other function. Was trying to be consistent.

@misteroneill

misteroneill Oct 19, 2017

Member

It's already documented for the other function. Was trying to be consistent.

This comment has been minimized.

@gkatsev

gkatsev Oct 19, 2017

Member

So it is. Carry on.

@gkatsev

gkatsev Oct 19, 2017

Member

So it is. Carry on.

@gkatsev gkatsev merged commit 85fe685 into master Oct 30, 2017

3 checks passed

continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gkatsev gkatsev deleted the hook-once branch Oct 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment