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: remove CaptionsSettingsMenuItem when TextTrackSetting is not inc… #5002

Merged
merged 9 commits into from
Mar 13, 2018

Conversation

vsoren
Copy link
Contributor

@vsoren vsoren commented Mar 7, 2018

Description

Removes the CaptionsSettingsMenuItem when TextTrackSettings is not included.

Fixes #4996

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

@ldayananda
Copy link
Contributor

Thanks for the PR @vsoren! Looks like you found some out of date tests-- if you take a look at the source code and then the tests that fail, you'll notice that '.vjs-texttrack-settings' should really be '.vjs-text-track-settings'

@ldayananda
Copy link
Contributor

In fact, specifically for the first test that fails , it doesn't seem like text-track-settings.test.js is right place for it. It should probably be removed and rewritten for the CaptionsSettingsMenuItem.

For this PR, I would be ok with removing that first test.

@ldayananda ldayananda added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Mar 8, 2018
Copy link
Contributor

@ldayananda ldayananda left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This looks good to me. Once another core contributor approves, we'll include it in an upcoming release.

@gkatsev
Copy link
Member

gkatsev commented Mar 12, 2018

So, after looking at this, I think we want to change the way the tests are. The tests look for the caption settings menu item and then "click" it to open the modal dialog. With this fix, we don't have a default caption settings menu item unless there are captions that are emulated. I'll make a PR for the test fixes.

@gkatsev
Copy link
Member

gkatsev commented Mar 12, 2018

I guess it just started failing because we weren't adding it unconditionally anymore.
@vsoren I made a PR against your branch (vsoren#1), once you merge that PR, we can merge this PR in!

@vsoren
Copy link
Contributor Author

vsoren commented Mar 13, 2018

@gkatsev The PR on my branch has been merged. :)

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Mar 13, 2018
@gkatsev gkatsev merged commit ba6a71e into videojs:master Mar 13, 2018
@gkatsev
Copy link
Member

gkatsev commented Mar 13, 2018

@vsoren thanks!

@vsoren vsoren deleted the captionssettings branch March 14, 2018 06:07
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.

Remove CaptionsSettingsMenuItem when TextTrackSettings is not included
4 participants