feat(emulated-tracks): add class to force cues to be center aligned #8625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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).
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
|
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. |
|
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. |
wseymour15
approved these changes
Mar 4, 2024
adrums86
approved these changes
Mar 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 thewidthto80%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