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

fix(FEC-8298): SOURCE_SELECTED event triggered before the UI ready when the source provided manually #129

Merged
merged 2 commits into from
Jan 24, 2019

Conversation

yairans
Copy link
Contributor

@yairans yairans commented Jun 11, 2018

Description of the Changes

postpone the sources config after the ui initialization

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

@yairans yairans self-assigned this Jun 11, 2018
@yairans yairans requested review from OrenMe and dan-ziv June 11, 2018 10:54
@yairans yairans changed the title fix(FEC-8298): SOURCE_SELECTED triggered before the UI ready when the… fix(FEC-8298): sourceselected triggered before the UI ready when the… Jun 11, 2018
@yairans yairans requested a review from odedhutzler June 11, 2018 11:00
@OrenMe OrenMe changed the title fix(FEC-8298): sourceselected triggered before the UI ready when the… fix(FEC-8298): sourceselected triggered before the UI ready when the source provided manually Jun 13, 2018
Copy link
Contributor

@OrenMe OrenMe left a comment

Choose a reason for hiding this comment

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

@yairans I think we better just differentiate this use case of sources provided in the setup call.
IMO we need to separate the sources from rest of config, call the player with all other settings and then call configure with the sources after UI is initialized.
UI relies on certain things in player as well so it's better the player is configured in advance.

Copy link
Contributor

@OrenMe OrenMe left a comment

Choose a reason for hiding this comment

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

@yairans I think we better just differentiate this use case of sources provided in the setup call.
IMO we need to separate the sources from rest of config, call the player with all other settings and then call configure with the sources after UI is initialized.
UI relies on certain things in player as well so it's better the player is configured in advance.

@yairans yairans changed the title fix(FEC-8298): sourceselected triggered before the UI ready when the source provided manually fix(FEC-8298): SOURCE_SELECTED event triggered before the UI ready when the source provided manually Jan 24, 2019
@@ -40,13 +40,16 @@ class KalturaPlayer extends FakeEventTarget {
constructor(options: KPOptionsObject) {
super();
this._eventManager = new EventManager();
this._localPlayer = loadPlayer(options);
const {sources} = options;
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if sources doesn't exist? what is the value?

Copy link
Contributor Author

@yairans yairans Jan 24, 2019

Choose a reason for hiding this comment

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

we have default sources object

this._logger = getLogger('KalturaPlayer' + Utils.Generator.uniqueId(5));
this._uiWrapper = new UIWrapper(this, options);
this._provider = new Provider(options.provider, __VERSION__);
this._playlistManager = new PlaylistManager(this, options);
this._playlistManager.configure(options.playlist);
Object.values(CoreEventType).forEach(coreEvent => this._eventManager.listen(this._localPlayer, coreEvent, e => this.dispatchEvent(e)));
this._localPlayer.configure({sources});
Copy link
Contributor

Choose a reason for hiding this comment

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

check if sources exist, don't call if it's null/undefined

OrenMe
OrenMe previously approved these changes Jan 24, 2019
@yairans yairans merged commit 44e8fb5 into master Jan 24, 2019
@yairans yairans deleted the FEC-8298 branch January 24, 2019 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants