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(player): add playerreset event #5335

Merged
merged 8 commits into from Nov 5, 2018
Merged

feat(player): add playerreset event #5335

merged 8 commits into from Nov 5, 2018

Conversation

@gstrat88
Copy link
Contributor

@gstrat88 gstrat88 commented Jul 21, 2018

Description

Added a reset event trigger to reset method. Use this event to reset a fluid player.

Specific Changes proposed

In reset() method a 'vjs_reset' event is being triggered. In fluid() method event listener is being initialialised.
Because the first time fluid() runs player is not "evented" yet, the listener is initialised after player is evented. Maybe it would be better to add the event as "delayed" and after player is "evented" init all "delayed" events.

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
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors
@gstrat88 gstrat88 changed the title Resetevent Reset event Jul 21, 2018
this.loadTech_(this.options_.techOrder[0], null);
this.techCall_('reset');
if (isEvented(this)) {
Copy link
Member

@gkatsev gkatsev Jul 23, 2018

Choose a reason for hiding this comment

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

I don't think this check is necessary because the Player will always has event handling.

Loading

Copy link
Contributor Author

@gstrat88 gstrat88 Jul 23, 2018

Choose a reason for hiding this comment

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

Player starts html5 tech and then resets test fails without it. It does not make a lot of sense though

Loading

@@ -768,8 +772,14 @@ class Player extends Component {

this.fluid_ = !!bool;

if (isEvented(this)) {
Copy link
Member

@gkatsev gkatsev Jul 23, 2018

Choose a reason for hiding this comment

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

same here, isEvented check isn't needed.

Loading

Copy link
Contributor Author

@gstrat88 gstrat88 Jul 23, 2018

Choose a reason for hiding this comment

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

Fluid is called first time before the player is evented.

Loading

src/js/player.js Outdated
this.loadTech_(this.options_.techOrder[0], null);
this.techCall_('reset');
if (isEvented(this)) {
this.trigger('vjs_reset');
Copy link
Member

@gkatsev gkatsev Jul 23, 2018

Choose a reason for hiding this comment

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

I think the best name for this may be playerreset. We already have precedent with the playerresize event.

Loading

Copy link
Contributor Author

@gstrat88 gstrat88 Jul 23, 2018

Choose a reason for hiding this comment

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

ok

Loading

src/js/player.js Outdated
@@ -404,6 +405,9 @@ class Player extends Component {
// Make this an evented object and use `el_` as its event bus.
evented(this, {eventBusKey: 'el_'});

if (this.fluid_) {
this.on('vjs_reset', this.updateStyleEl_);
Copy link
Member

@gkatsev gkatsev Jul 23, 2018

Choose a reason for hiding this comment

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

so, the idea is to rerun updateStyleEl_ when reset is called? Makes sense to me.

Loading

Copy link
Member

@gkatsev gkatsev Jul 23, 2018

Choose a reason for hiding this comment

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

so, fluid gets called in createEl so, I'm not sure we actually need this here. We can probably get away with just the piece inside the fluid method.

Loading

Copy link
Contributor Author

@gstrat88 gstrat88 Jul 23, 2018

Choose a reason for hiding this comment

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

Fluid is called first time before createEl, player is not evented yet. So no listener. Thats why the listener need to be added after createEl. I will take a look if first time fluid can move after player evented, but a delayed listener also seems a viable solution for me.

Loading

Copy link
Member

@gkatsev gkatsev Jul 23, 2018

Choose a reason for hiding this comment

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

I see, 🤔

Loading

…vented is inited, or run immediately when object is evented.
@stale
Copy link

@stale stale bot commented Oct 3, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Loading

@stale stale bot added the outdated label Oct 3, 2018
@gkatsev gkatsev removed the outdated label Oct 3, 2018
gkatsev
gkatsev approved these changes Nov 5, 2018
Copy link
Member

@gkatsev gkatsev left a comment

LGTM

Loading

@gkatsev gkatsev changed the title Reset event feat(player): add playerreset event Nov 5, 2018
@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 5, 2018

The test failures are unrelated, merging anyway. We will not do a release if tests continue failing.

Loading

@gkatsev gkatsev merged commit 0e5442f into videojs:master Nov 5, 2018
1 of 2 checks passed
Loading
@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 5, 2018

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants