Feature/reset player ui #4683 #5684

reeckset · 2018-12-13T15:51:40Z

Description

Changes

Adds a method in player for resetting its UI (progress bar, volume, playback rate, remaining time display). This is achieved through 3 other new methods to keep a more modular structure. Also created unit tests for this feature, which are passing at the moment.

Besides the mentioned changes, class RemainingTimeDisplay and the player's tech reset unit tests were also modified to fully support the new added changes.

Requirements Checklist

Feature implemented
Tested in multiple browsers
Test cases passed
Reviewed by Two Core Contributors

welcome · 2018-12-13T15:51:44Z

💖 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.

gkatsev

We also have #5676 in the pipeline, this PR would need to be fixed up afterwards, though, I'd be happy to help fix any merge issues that occur.

gkatsev · 2018-12-13T16:41:28Z

src/js/player.js

+   */
+  resetProgressBar() {
+    this.currentTime(0);
+    if (isNaN(this.duration())) {


as part of #5348, duration can now be NaN, and it's probably more accurate as well.

Thank you for the input. The reason why i added this is that when the duration is NaN, the remaining-time-display does not change or update. Hence setting it to 0. Is there any other way to fix this?

I think it's more correct to set the duration to NaN. We should call duration display's update directly here instead.

@gkatsev the thing is if I set it to NaN the remaining time display does not change. Even when calling duration display's update directly.

From our analysis, PR #5348 would make setting the duration to NaN update the duration display. Therefore, if this PR follows #5348, setting the duration to NaN will trigger the update in the remaining time display

#5348 is in 7.4.0 and 7.4 is about to be promoted to latest. I can confirm that calling updateContent on the duration display after setting the duration to NaN will change the duration display to be -:-. However, the remaining time display does a null check on the duration. This probably needs to be updated to verify that the duration is a number. So,

video.js/src/js/control-bar/time-controls/remaining-time-display.js

Lines 64 to 66 in f0ba1f5

if (!this.player_.duration()) {

return;

}

probably should be something like:

if (typeof this.player_.duration() !== 'number') { return; }

Allright, we will also add that change to the PR then. It should be submitted in a couple of hours. Thank you for all the help :)

@gkatsev I think it's done. Anything else please let me know!

gkatsev · 2018-12-13T16:42:00Z

src/js/player.js

+   * Reset Control Bar's UI by calling sub-methods that reset
+   * all of Control Bar's components
+   */
+  resetControlBarUI() {


these methods should be suffixed with an _ to show that they're "private" or shouldn't be used directly outside of Video.js.

gkatsev · 2018-12-13T16:45:14Z

Also, package-lock changes should probably be backed out.

xRuiAlves · 2018-12-13T18:37:43Z

Thank you for the input, we will fix these issues ASAP!

misteroneill

Thanks for this PR! It's extremely helpful!

src/js/player.js

…r-UI

reeckset · 2018-12-17T13:34:41Z

@gkatsev I think I solved all the problems with this pull request, with the exception of the NaN one because of the above mentioned behaviour. Any other issues with it please let me know so it can be accepted.

xRuiAlves · 2018-12-18T20:51:28Z

@misteroneill we have implemented the changes you requested. Do you think de PR would eligible to be accepted?

src/js/player.js

gkatsev · 2018-12-18T20:52:12Z

src/js/player.js

+   */
+  resetProgressBar() {
+    this.currentTime(0);
+    if (isNaN(this.duration())) {


I think it's more correct to set the duration to NaN. We should call duration display's update directly here instead.

gkatsev · 2018-12-18T20:52:58Z

src/js/player.js

+      this.duration(0);
+    }
+    this.tech_.trigger({ type: 'durationchange', target: this.tech_, manuallyTriggered: true });
+    this.tech_.trigger({ type: 'timeupdate', target: this.tech_, manuallyTriggered: true });


This PR will likely follow #5676, I'm not sure that we'd need to trigger timeupdate with the changes in #5676.

test/unit/reset-ui.test.js

…ssues

gkatsev · 2018-12-19T20:41:18Z

I guess the duration automatically gets reset to NaN as part of the reset process right now?

Also, can resetControlBarUI be made "private" by appending the _ suffix? https://github.com/videojs/video.js/pull/5684/files#r241475268

However, this is more or less good to go.

reeckset · 2018-12-19T21:04:12Z

@gkatsev yes it does. I just added the suffix. Ready to go!

gkatsev

Approving currently but this will likely need changes when the other reset PR is merged in.
Thanks!

xRuiAlves · 2018-12-20T07:56:27Z

Thanks @gkatsev ! Any changes we can help in, let us know

gkatsev · 2019-01-03T20:17:53Z

Looks like there wasn't much conflicts after merging in #5676. I've resolved the conflict via github.

updated

misteroneill

Thanks!

welcome · 2019-01-08T19:15:55Z

Congrats on merging your first pull request! 🎉🎉🎉

gkatsev · 2019-01-08T19:16:06Z

Thanks all!

matt-snider · 2019-02-16T21:03:22Z

@reeckset @imnotteixeira @xRuiAlves I just tried this out in v7.5.0. Looks good, but one thing I noticed is that the loading spinner isn't hidden. So for example, if the content was still loading when reset() was called, it will remain there indefinitely. I saw that there is also #5629 which is open so maybe it's meant to be addressed there?

reeckset added 3 commits November 29, 2018 13:24

Finished reset of progress bar and playback rate

73c109c

Added volume reset

bf5b0b8

Fixed remaining time reset, added unit tests

0ea44eb

gkatsev reviewed Dec 13, 2018

View reviewed changes

misteroneill previously requested changes Dec 14, 2018

View reviewed changes

src/js/player.js Outdated Show resolved Hide resolved

reeckset added 4 commits December 17, 2018 12:55

Changed private method names to have _ suffix

69a5bbf

Moved UI reset method call to before playerreset event is triggered

7a0338c

Rolled package-lock back

c6bc3d9

Merge remote-tracking branch 'origin/master' into feature/reset-playe…

134cce4

…r-UI

gkatsev mentioned this pull request Dec 18, 2018

Fix: duration display when changing video duration or source #5693

Closed

4 tasks

gkatsev reviewed Dec 18, 2018

View reviewed changes

gkatsev added the patch This PR can be added to a patch release. label Dec 18, 2018

reeckset added 2 commits December 19, 2018 12:21

Removed durationchange trigger call in resetProgressBar, fixed test i…

93411a6

…ssues

Changed duration to NaN upon reset

4987953

xRuiAlves mentioned this pull request Dec 19, 2018

Refactor/loading spinner show hide #5629

Closed

4 tasks

Changed resetControlBarUI to resetControlBarUI_

05ea374

Updated tests to call resetControlBarUI_ (with suffix)

d0d09b5

gkatsev approved these changes Dec 20, 2018

View reviewed changes

Merge branch 'master' into feature/reset-player-UI

9df7e79

gkatsev added the needs: LGTM Needs one or more additional approvals label Jan 3, 2019

misteroneill approved these changes Jan 8, 2019

View reviewed changes

misteroneill removed the needs: LGTM Needs one or more additional approvals label Jan 8, 2019

gkatsev merged commit 175f773 into videojs:master Jan 8, 2019

amtins mentioned this pull request Feb 26, 2023

Fix/reset progress bar #8160

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/reset player ui #4683 #5684

Feature/reset player ui #4683 #5684

reeckset commented Dec 13, 2018 •

edited by gkatsev

Loading

welcome bot commented Dec 13, 2018

gkatsev left a comment

gkatsev Dec 13, 2018

reeckset Dec 17, 2018

gkatsev Dec 18, 2018

reeckset Dec 19, 2018

xRuiAlves Dec 19, 2018

gkatsev Dec 19, 2018

xRuiAlves Dec 19, 2018

reeckset Dec 19, 2018

gkatsev Dec 13, 2018

gkatsev commented Dec 13, 2018

xRuiAlves commented Dec 13, 2018

misteroneill left a comment

reeckset commented Dec 17, 2018

xRuiAlves commented Dec 18, 2018

gkatsev Dec 18, 2018

gkatsev Dec 18, 2018

gkatsev commented Dec 19, 2018

reeckset commented Dec 19, 2018

gkatsev left a comment

xRuiAlves commented Dec 20, 2018

gkatsev commented Jan 3, 2019

misteroneill left a comment

welcome bot commented Jan 8, 2019

gkatsev commented Jan 8, 2019

matt-snider commented Feb 16, 2019

Feature/reset player ui #4683 #5684

Feature/reset player ui #4683 #5684

Conversation

reeckset commented Dec 13, 2018 • edited by gkatsev Loading

Description

Changes

Requirements Checklist

welcome bot commented Dec 13, 2018

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Dec 13, 2018

xRuiAlves commented Dec 13, 2018

misteroneill left a comment

Choose a reason for hiding this comment

reeckset commented Dec 17, 2018

xRuiAlves commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Dec 19, 2018

reeckset commented Dec 19, 2018

gkatsev left a comment

Choose a reason for hiding this comment

xRuiAlves commented Dec 20, 2018

gkatsev commented Jan 3, 2019

misteroneill left a comment

Choose a reason for hiding this comment

welcome bot commented Jan 8, 2019

gkatsev commented Jan 8, 2019

matt-snider commented Feb 16, 2019

reeckset commented Dec 13, 2018 •

edited by gkatsev

Loading