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

feat: Add 'beforepluginsetup' event and named plugin setup events (e.g. 'pluginsetup:foo') #4255

Merged
merged 3 commits into from
May 11, 2017

Conversation

misteroneill
Copy link
Member

@misteroneill misteroneill commented Apr 5, 2017

This adds a beforepluginsetup event as well as beforepluginsetup:$name and pluginsetup:$name events.

The drive behind this is improving the ability for people to make cross-plugin dependencies in a more robust manner.

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

Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also want events for after setup, and dispose?

src/js/plugin.js Outdated
* A plugin event hash.
*
* @param {Boolean} [before]
* If true, modifies the events to be "before" events.
Copy link
Contributor

@brandonocasey brandonocasey Apr 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to have an example, as this confused me at first. Maybe: 'I.E pluginsetup -> beforepluginsetup'

//
// The only potentially counter-intuitive thing here is the `instance` in
// the "pluginsetup" event is the value returned by the `plugin` function.
triggerSetupEvent(this, {name, plugin, instance: null}, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm up in the air about wether we should pass data along with the event. I understand why we would need to but, it still goes against most of the practices that we have for other events. Anyone else want to chime in here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's already the case for pluginsetup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, I guess this is just what we have to do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I do know some plugins do return something from the method, so, providing it is nice.

@misteroneill
Copy link
Member Author

We could potentially add after events at some point, but, similar to the modal events, the "unprefixed" events are already kind of after events as it is.

@misteroneill
Copy link
Member Author

Also, I plan to add tests for this at some point this week.

@gkatsev gkatsev merged commit 0a19cf0 into videojs:master May 11, 2017
@misteroneill misteroneill deleted the feat/pluginsetup-events branch October 2, 2017 20:38
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.

3 participants