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

feat(emulated-tracks): add class to force cues to be center aligned #8625

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Mar 1, 2024

videojs/http-streaming#1408 updated 608 captions to default to be left aligned. This may be unwanted by some folks and we should provide an easier way to force them to be centered. This PR adds a player level class that will override the text alignment to be center. It also overrides the width to 80% because otherwise the cue box isn't set up correctly to be 10% from the right of the display area (a side effect of hardcoding a width value and using inset in the generation of the cues).

Description

Please describe the change as necessary.
If it's a feature or enhancement please be as detailed as possible.
If it's a bug fix, please link the issue that it fixes or describe the bug in as much detail.

Specific Changes proposed

Please list the specific changes involved in this pull request.

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

videojs/http-streaming#1408 updated 608 captions to default to be left aligned. This may be unwanted by some folks and we should provide an easier way to force them to be centered.
This PR adds a player level class that will override the text alignment to be `center`. It also overrides the `width` to `80%` because otherwise the cue box isn't set up correctly to be 10% from the right of the display area (a side effect of hardcoding a width value and using inset in the generation of the cues).
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.71%. Comparing base (d7757d8) to head (ca1ec65).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8625   +/-   ##
=======================================
  Coverage   82.71%   82.71%           
=======================================
  Files         113      113           
  Lines        7636     7636           
  Branches     1835     1835           
=======================================
  Hits         6316     6316           
  Misses       1320     1320           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wseymour15
Copy link
Contributor

This looks like a fine change to me, assuming that this class setting will be optional. We made this change because according to the spec: "CEA-608 does not distinguish between different alignment, so all captions are left aligned and require a WebVTT align cue setting to override the default middle alignment".

We interpreted this to mean by default, 608 captions should be left aligned.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 4, 2024

Yup, it's purely an opt-in class to make it easier to center. See some discussion in slack. I agree that the updated default is the correct one.

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.

None yet

4 participants