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: change media #61

Merged
merged 10 commits into from
Oct 15, 2017
Merged

feat: change media #61

merged 10 commits into from
Oct 15, 2017

Conversation

dan-ziv
Copy link
Contributor

@dan-ziv dan-ziv commented Sep 28, 2017

Description of the Changes

UI handling of change media.

Needs to be merge after playkit-js PR will be merged.

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

this.props.updatePrePlayback(false);
this.props.removePlayerClass('pre-playback');
});
this.player.addEventListener(this.player.Event.PLAY, this._hidePrePlayback.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

use ()=>this. _hidePrePlayback()

@@ -146,6 +139,30 @@ class PrePlaybackPlayOverlay extends BaseComponent {
</div>
)
}

_addBindings(): void {
this.player.addEventListener(this.player.Event.CHANGE_SOURCE_ENDED, this._onChangeSourceEnded.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

missing JSDoc.
Beeter discuss with @dvh91 before adding _addBindings convention, as Preact might have something else

Copy link

Choose a reason for hiding this comment

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

You can use componentWillMount function from the component lifecycle and get rid of the addBindings function.

this.player.addEventListener(this.player.Event.CHANGE_SOURCE_ENDED, this._onChangeSourceEnded.bind(this));
}

_onChangeSourceEnded(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing JSDoc

@@ -33,6 +33,10 @@ class EngineConnector extends BaseComponent {
componentDidMount() {
const TrackType = this.player.Track;

this.player.addEventListener(this.player.Event.CHANGE_SOURCE_ENDED, () => {
this.props.updatePlayerPoster(this.player.poster);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed? for scrubber preview? @dvh91 if this is the case, as with other places where you fetch the poster for this, then we should extract this and have the UI config get the dedicated scrubber config(e.g. the "backed" url and number of segments and width)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are not auto playing the next media we want to see the updated poster

@dan-ziv dan-ziv closed this Oct 9, 2017
@dan-ziv dan-ziv reopened this Oct 9, 2017
@dan-ziv dan-ziv merged commit 6c5c2c4 into master Oct 15, 2017
@dan-ziv dan-ziv deleted the change-media branch October 15, 2017 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants