Add audio description example to VideoPlayer docs #882

joshfarrant · 2025-01-08T10:06:03Z

Summary

Adds an audio description example to the VideoPlayer documentation, and fixes styling of custom play icon example.

List of notable changes:

Added an audio description example to the VideoPlayer documentation
Changed the fill of the custom play icon in the VideoPlayer documentation. The same change was made to the story in Fix VideoPlayer accessibility issues #703 — specifically this file — but I missed the example in the docs at that point.

What should reviewers focus on?

Make sure you're happy with the example and phrasing.

Steps to test:

Take a look at the updated docs

Supporting resources (related issues, external links, etc):

Closes https://github.com/github/primer/issues/3771

Contributor checklist:

All new and existing CI checks pass
Tests prove that the feature works and covers both happy and unhappy paths
Any drop in coverage, breaking changes or regressions have been documented above
UI Changes contain new visual snapshots (generated by adding update snapshots label to the PR)
All developer debugging and non-functional logging has been removed
Related issues have been referenced in the PR description

Reviewer checklist:

Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
Check that tests prove the feature works and covers both happy and unhappy paths
Check that there aren't other open Pull Requests for the same update/change

changeset-bot · 2025-01-08T10:06:08Z

⚠️ No Changeset found

Latest commit: 0a7d88a

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

github-actions · 2025-01-08T10:07:46Z

🟢 No design token changes found

github-actions · 2025-01-08T10:16:25Z

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

danielguillan

Looks good, thank you!

rezrah · 2025-01-08T12:52:10Z

apps/docs/content/components/VideoPlayer.mdx

+<Stack gap="normal">
+  <VideoPlayer title="GitHub media player">
+    <VideoPlayer.Source src="/brand/assets/example.mp4" />
+    <VideoPlayer.Track src="/brand/assets/example.vtt" default />
+  </VideoPlayer>
+  <Link href="#">Watch with audio description</Link>
+</Stack>


The link hover state is taking up the full width of the container. Quick fix below 👇

Suggested change

<Stack gap="normal">

<VideoPlayer title="GitHub media player">

<VideoPlayer.Source src="/brand/assets/example.mp4" />

<VideoPlayer.Track src="/brand/assets/example.vtt" default />

</VideoPlayer>

<Link href="#">Watch with audio description</Link>

</Stack>

<Stack gap="normal" alignItems="center">

<VideoPlayer title="GitHub media player">

<VideoPlayer.Source src="/brand/assets/example.mp4" />

<VideoPlayer.Track src="/brand/assets/example.vtt" default />

</VideoPlayer>

<Link href="#">Watch with audio description</Link>

</Stack>

Also could this be a button instead of a link? I'm not sure what this is supposed to do right now as clicking it scrolls me back to top of page due to it being a link with # href.

The intention here is to demonstrate the second pattern described above.

or via a link to an alternate audio-described version

As we don't have an audio-described version of this video I opted to just use a href of "#". If there's another approach you'd prefer just let me know.

Good spot on the width issue too, that's now fixed.

Gotcha, thanks @joshfarrant. Could we instead use a no-op link, or open an alert saying something like "Audio description should begin playing" (Assuming that's what's supposed to happen?). I think the scrolling is a quite distracting right now, what do you think?

I've not seen this audio description link on any of our marketing videos, so wonder how it's supposed to look. We should ask Site at some point for guidance on whether it should be button or link. Until then it's fine to keep as a link.

Happy to update it, but just want to make sure we're on the same page about the intended behaviour here.

The intention is that the text "Watch with audio description" will link to an alternative video on some other page; that video will have an audio description embedded in it. That's why this can only be a link and not a button.

So if we're keeping it as a link then should we just keep it as href="#" since we do that in quite a few other places in the docs already?

I've asked around and I can't find any self-hosted videos which have an optional audio description, so there might not be an existing pattern for this. My goal here is to take the simplest possible approach and just ask consumers to provide a link to an audio described version.

Hopefully that makes sense. If you think that's not clear from my changes to the docs then lmk too. If it's preferable then I could remove the example altogether if you think it's causing confusion, then we can just leave it up to consumers to decide how they want to provide the link.

Thanks Josh, makes sense to keep it a link. How about we add an external link icon next to it then, to make it obvious before someone clicks on it, that it should eventually go to an external location. Otherwise it still feels both a little broken with the scrolling and confusing on should happen.

<Stack gap="normal" alignItems="center"> <VideoPlayer title="GitHub media player"> <VideoPlayer.Source src="/brand/assets/example.mp4" /> <VideoPlayer.Track src="/brand/assets/example.vtt" default /> </VideoPlayer> <Stack direction="horizontal" gap={12} padding="none" alignItems="center"> <Link href="#" arrowDirection="none">Watch with audio description</Link> <LinkExternalIcon size={16} /> </Stack> </Stack>

Should produce:

Works for me, thanks!

Just spotted a style issue on hover again, let me fix that and I'll ping you when it's ready

@rezrah The Link underline issue is going to be fixed in #887 to save cluttering this PR. For the moment that means it looks like this in the docs deployed with this PR

Once #887 goes in the underline will be fixed.

That should tick off the last of the feedback for this PR, let me know if you spot anything else though

apps/docs/content/components/VideoPlayer.mdx

joshfarrant temporarily deployed to github-pages January 8, 2025 10:12 — with GitHub Actions Inactive

joshfarrant marked this pull request as ready for review January 8, 2025 10:17

joshfarrant assigned rezrah and danielguillan Jan 8, 2025

danielguillan approved these changes Jan 8, 2025

View reviewed changes

joshfarrant requested a review from rezrah January 8, 2025 12:47

joshfarrant unassigned danielguillan and rezrah Jan 8, 2025

rezrah reviewed Jan 8, 2025

View reviewed changes

apps/docs/content/components/VideoPlayer.mdx Outdated Show resolved Hide resolved

joshfarrant requested a review from rezrah January 8, 2025 16:21

joshfarrant temporarily deployed to github-pages January 8, 2025 16:28 — with GitHub Actions Inactive

joshfarrant temporarily deployed to github-pages January 9, 2025 12:35 — with GitHub Actions Inactive

joshfarrant added 5 commits January 10, 2025 08:46

set fill of custom play icon

2490689

add audio description example to VideoPlayer docs

1b0dd22

fix example

5b90c47

prevent link from taking 100% width

d98d597

add external link icon

0a7d88a

joshfarrant force-pushed the joshfarrant/videoplayer-audio-description branch from 9e6db68 to 0a7d88a Compare January 10, 2025 08:47

joshfarrant deployed to github-pages January 10, 2025 08:54 — with GitHub Actions View deployment

rezrah approved these changes Jan 13, 2025

View reviewed changes

joshfarrant merged commit ed322ea into main Jan 13, 2025
17 checks passed

joshfarrant deleted the joshfarrant/videoplayer-audio-description branch January 13, 2025 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add audio description example to VideoPlayer docs #882

Add audio description example to VideoPlayer docs #882

joshfarrant commented Jan 8, 2025 •

edited

Loading

changeset-bot bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

danielguillan left a comment

rezrah Jan 8, 2025

rezrah Jan 8, 2025 •

edited

Loading

joshfarrant Jan 8, 2025

rezrah Jan 8, 2025

joshfarrant Jan 9, 2025 •

edited

Loading

rezrah Jan 9, 2025

joshfarrant Jan 9, 2025

joshfarrant Jan 9, 2025

joshfarrant Jan 10, 2025

Add audio description example to VideoPlayer docs #882

Add audio description example to VideoPlayer docs #882

Conversation

joshfarrant commented Jan 8, 2025 • edited Loading

Summary

List of notable changes:

What should reviewers focus on?

Steps to test:

Supporting resources (related issues, external links, etc):

Contributor checklist:

Reviewer checklist:

changeset-bot bot commented Jan 8, 2025 • edited Loading

⚠️ No Changeset found

github-actions bot commented Jan 8, 2025 • edited Loading

🟢 No design token changes found

github-actions bot commented Jan 8, 2025 • edited Loading

🟢 No visual differences found

danielguillan left a comment

Choose a reason for hiding this comment

rezrah Jan 8, 2025

Choose a reason for hiding this comment

rezrah Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

joshfarrant Jan 8, 2025

Choose a reason for hiding this comment

rezrah Jan 8, 2025

Choose a reason for hiding this comment

joshfarrant Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

rezrah Jan 9, 2025

Choose a reason for hiding this comment

joshfarrant Jan 9, 2025

Choose a reason for hiding this comment

joshfarrant Jan 9, 2025

Choose a reason for hiding this comment

joshfarrant Jan 10, 2025

Choose a reason for hiding this comment

joshfarrant commented Jan 8, 2025 •

edited

Loading

changeset-bot bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

rezrah Jan 8, 2025 •

edited

Loading

joshfarrant Jan 9, 2025 •

edited

Loading