-
Notifications
You must be signed in to change notification settings - Fork 2
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(FEC-10783): create BP plugin #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yuvalke please look at the IMA and IMA DAI engine proxy samples.
The setMedia usage is invalid in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove .DS_Store and add to .gitignore list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add GH issue and PR templates
src/bp-engine-decorator.js
Outdated
* @param {BroadPeak} plugin - The broadpeak plugin. | ||
* @implements {IEngineDecorator} | ||
*/ | ||
class BpEngineDecorator implements IEngineDecorator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BroadPeakEngineDecorator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept it with BP instead of BroadPeak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's your call, but naming usually should be consistent and not change from file to file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's, also middleware use the shortcut and once it was broadpeak smartlib it also used BP instead broadpeak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yuvalke LGTM but need to update README to integration with kaltura player and not playkit (plugins are now one level up -see here https://github.com/kaltura/playkit-js-airplay)
The plugin requires [PlayKit JS Player] to be loaded first. | ||
|
||
[playkit js player]: https://github.com/kaltura/playkit-js | ||
The plugin requires [Kaltura Player JS] to be loaded first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title link is broken. need to add
[kaltura player js]: https://github.com/kaltura/kaltura-player-js
enable to use of broadpeak smartlib as a plugin which replacing our source by source we're getting from broadpeak smartlib.