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: current time tooltip does not update #6445

Merged
merged 2 commits into from
Feb 18, 2020

Conversation

marcosmx
Copy link
Contributor

Description

The progress bar tooltip get stuck and don't update the time as the video plays. It only respond to mouse movements and reflects the time in the current time tool tip.

Specific Changes proposed

Add a reference for the playProgressBar component at seekBar to trigger the update action on every playing and timeupdate event

@welcome
Copy link

welcome bot commented Feb 13, 2020

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

It looks like this essentially restores this code block from the PR that caused the regression.

Does this.bar not work anymore for some reason?

Regardless, we should probably null check it before calling update(). Generally, it's a good practice not to assume a component exists (the original code had if (this.bar)).

@marcosmx
Copy link
Contributor Author

@misteroneill
I didn't notice about the presence of property class bar, that's was the reason for not used but I changed the code to use it and validate the null state of it.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Is there a sensible way to add a test here? If none exists, it may make sense to add some initial progress-control/seek-bar tests.

@gkatsev gkatsev merged commit e6c03c7 into videojs:master Feb 18, 2020
@welcome
Copy link

welcome bot commented Feb 18, 2020

Congrats on merging your first pull request! 🎉🎉🎉

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.

4 participants