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: menu sizing when using longer caption labels #5228

Merged
merged 2 commits into from
Jun 11, 2018
Merged

fix: menu sizing when using longer caption labels #5228

merged 2 commits into from
Jun 11, 2018

Conversation

bcdarius
Copy link
Contributor

@bcdarius bcdarius commented Jun 5, 2018

Description

Proposed solution for #4758

Currently, if longer captions labels are used the SubsCaps menu gets all messed up in IE11/Edge. This occurs due to the menu size is not adjusted correctly not taking into consideration the CC icon. This, as a result, causes the captions items not to fit in the menu and because they end up too wide and the menu has overflow: auto style, the horizontal scrollbars show up and cause the menu not to render correctly.

Specific Changes proposed

Do not position the CC icon absolutely.

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

@gkatsev
Copy link
Member

gkatsev commented Jun 5, 2018

This actually works, though, it's not quite aligned properly with the text anymore for a short label, adding bottom-margin: -0.1em; seems to fix it. Thoughts?

@bcdarius
Copy link
Contributor Author

bcdarius commented Jun 6, 2018

Haven't noticed this until you pointed out. Aligning the icon with negative margin-bottom value sounds a reasonable solution to me.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

This seems good to me.

@gkatsev
Copy link
Member

gkatsev commented Jun 6, 2018

@OwenEdwards hey can you take a look at this? Especially wrt your PR #4599.

@OwenEdwards
Copy link
Member

This change, rolled into #4599, fixes that PR completely! So this LGTM.

OwenEdwards added a commit to OwenEdwards/video.js that referenced this pull request Jun 7, 2018
@gkatsev gkatsev merged commit 002d701 into videojs:master Jun 11, 2018
gkatsev pushed a commit that referenced this pull request Jun 11, 2018
Currently, if longer captions labels are used the SubsCaps menu gets all messed up in IE11/Edge. This occurs due to the menu size is not adjusted correctly not taking into consideration the CC icon. This, as a result, causes the captions items not to fit in the menu and because they end up too wide and the menu has overflow: auto style, the horizontal scrollbars show up and cause the menu not to render correctly. Therefore, don't position the caption label absolutely.

Fix #4758.
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