Skip to content

Conversation

@geoffrich
Copy link
Member

This PR enables the video captions by default and adds a toggle to turn them on and off. We don't have a ton of styling flexibility, but I did what I could. The visual "enabled" state of the toggle is just a text underline.

I applied the same "unused" styling as the sound toggle, to make it more obvious that it's available.

I wasn't sure where to get an icon for the CC toggle so it's just text for now (which seems to cause some alignment issues with the sound toggle in firefox). I think it looks okay, but happy to swap out for an icon if someone can provide an SVG.

We could disable the captions by default if necessary, but imo it's nice having them on by default so people can understand the video without needing to turn on sound or hunt for the CC button. Especially since we autoplay the video and there's no way to seek at the moment - this helps people not miss the beginning.

Screenshots of captions on different browsers

Chrome

image

Safari

image

Firefox

image

Mobile screen width (Chrome)

image

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Dec 10, 2022

⚠️ No Changeset found

Latest commit: da077d6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

*/
function set_cue_position(node) {
const cues = node.track.cues;
if (!cues) return;
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this works because the .vtt file is loaded by the time the JS runs (basically the inverse of the problem in onMount here), but it could presumably happen in the opposite order. Is there an event we can listen for if cues doesn't exist yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call - I was able to listen to the <track>'s load event if we don't have cues yet. I tested it by wrapping the <track> in an {#if browser} block and it seemed to work. 922677f

@benmccann
Copy link
Member

nice! the captions are really great to have. it's nice that I can get the value out of the video while it's muted and I also think it will grab the attention of visitors who might otherwise skip over it

@Rich-Harris
Copy link
Member

  • added a pair of icons
  • removed the opacity: 1 on focus, because we now have the outline, and having the opacity: 1 persist after you've interacted with the button next to a button that doesn't have opacity: 1 looks weird
  • synced the unused class between the two buttons — as soon as you interact with one, they both fade into the background (the reason they begin unfaded is so that you know there are buttons, but that's no longer necessary once you've interacted with one of them)

amazing work getting the subtitles to look good, i was very much resigned to them not doing so. i think having them visible when you first hit the page is a huge win.

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