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

Tight coupling with controlBar component #7689

Closed
try2beth3b3st opened this issue Mar 23, 2022 · 5 comments · Fixed by #7692
Closed

Tight coupling with controlBar component #7689

try2beth3b3st opened this issue Mar 23, 2022 · 5 comments · Fixed by #7692

Comments

@try2beth3b3st
Copy link
Contributor

Description

We have tight coupling in player.js Class Player method resetControlBarUI_(), when using reset() method.
ControlBar component may be not exists or exists on another nesting level..
image

Steps to reproduce

Use player.reset() method, when in player object there is no controlBar child/component at property "children"

Results

Expected

Reset player instance

Actual

We have error

Error output

image

Additional Information

versions

videojs

7.17.1

browsers

all

OSes

all

(Sorry for my bad english)

@welcome
Copy link

welcome bot commented Mar 23, 2022

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
To help make it easier for us to investigate your issue, please follow the contributing guidelines.

@gkatsev
Copy link
Member

gkatsev commented Mar 23, 2022

Having a control bar is definitely the default use-case. We probably don't test enough with trying to disable default components.
Looks like it's because it's trying to dereference from an undefined object. The solution is likely to add a default value on the line where durationDisplay and remainingTimeDisplay are being destructured.

const  { durationDisplay, remainingTimeDisplay } = this.controlBar ?? {};

if our build doesn't like ?? we can continue using ||.

Are you interesting in submitting a PR?

@try2beth3b3st
Copy link
Contributor Author

try2beth3b3st commented Mar 24, 2022

Having a control bar is definitely the default use-case. We probably don't test enough with trying to disable default components. Looks like it's because it's trying to dereference from an undefined object. The solution is likely to add a default value on the line where durationDisplay and remainingTimeDisplay are being destructured.

const  { durationDisplay, remainingTimeDisplay } = this.controlBar ?? {};

if our build doesn't like ?? we can continue using ||.

Are you interesting in submitting a PR?

Yes. But i can't push fix for PR now (Haven't permissions for push).
image

@mister-ben
Copy link
Contributor

You'll need to create your own fork of the repo, make your changes in a branch, then a pull request to propose the changes from your branch are added to Video.js. There's a quick walkthrough here: https://opensource.com/article/19/7/create-pull-request-github

try2beth3b3st pushed a commit to try2beth3b3st/video.js that referenced this issue Mar 24, 2022
Added condition to resetProgressBar_ method
try2beth3b3st pushed a commit to try2beth3b3st/video.js that referenced this issue Mar 24, 2022
@try2beth3b3st
Copy link
Contributor Author

Done

try2beth3b3st pushed a commit to try2beth3b3st/video.js that referenced this issue Mar 25, 2022
gkatsev pushed a commit that referenced this issue May 4, 2022
Fixes #7689

Co-authored-by: alex <try2betheb3st@gmail.com>
Co-authored-by: Pat O'Neill <pgoneill@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 3, 2022
edirub pushed a commit to edirub/video.js that referenced this issue Jun 8, 2023
Fixes videojs#7689

Co-authored-by: alex <try2betheb3st@gmail.com>
Co-authored-by: Pat O'Neill <pgoneill@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants