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: tight controlbar coupling (#7689) #7692

Merged
merged 3 commits into from
May 4, 2022

Conversation

try2beth3b3st
Copy link
Contributor

@try2beth3b3st try2beth3b3st commented Mar 24, 2022

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.
Fixes #7689

Specific Changes proposed

Added condition

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 (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

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.

Nice, these missing null checks are too common! Would you mind adding a test for this case?

@try2beth3b3st
Copy link
Contributor Author

Nice, these missing null checks are too common! Would you mind adding a test for this case?

Done..

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #7692 (56baa81) into main (c5a0376) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7692      +/-   ##
==========================================
+ Coverage   80.89%   80.91%   +0.01%     
==========================================
  Files         116      116              
  Lines        7455     7455              
  Branches     1806     1807       +1     
==========================================
+ Hits         6031     6032       +1     
+ Misses       1424     1423       -1     
Impacted Files Coverage Δ
src/js/player.js 88.37% <100.00%> (ø)
src/js/component.js 94.17% <0.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5a0376...56baa81. Read the comment docs.

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.

One minor quibble, otherwise looks good!

test/unit/player.test.js Show resolved Hide resolved
Co-authored-by: Pat O'Neill <pgoneill@gmail.com>
Copy link
Contributor

@mister-ben mister-ben left a comment

Choose a reason for hiding this comment

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

Tested manually with a player with controlBar: false, looks good.

@misteroneill misteroneill added confirmed patch This PR can be added to a patch release. labels Apr 7, 2022
@try2beth3b3st
Copy link
Contributor Author

@misteroneill Excuse me, when to expect patch with this fix?)

@gkatsev gkatsev merged commit 7e2b9ec into videojs:main May 4, 2022
@welcome
Copy link

welcome bot commented May 4, 2022

Congrats on merging your first pull request! 🎉🎉🎉

edirub pushed a commit to edirub/video.js that referenced this pull request 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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tight coupling with controlBar component
4 participants