Skip to content

Minor self-hosted video refactor#15732

Merged
domlander merged 1 commit intomainfrom
doml/sh-video-refactor
Apr 21, 2026
Merged

Minor self-hosted video refactor#15732
domlander merged 1 commit intomainfrom
doml/sh-video-refactor

Conversation

@domlander
Copy link
Copy Markdown
Contributor

@domlander domlander commented Apr 21, 2026

What does this change?

This is a no-op change.

  • We never want to show the progress bar or the icons if the player has yet to begin playing video, so this logic is moved up.
  • Logic to update the current time in state when the onTimeUpdate event fires on the video is moved up to the island component.
  • When we want to change the time of the video, we usually do two things: update the time on the video itself and update our representation of the current time in state. A helper function that does both has been created.

Why?

  • To keep logic in the island instead of the player.
  • To keep the code tidy and prepare for upcoming changes.

How to test?

Run storybook locally. Locate the SelfHostedVideo stories

@domlander domlander changed the title minor self-hosted video refactor Minor self-hosted video refactor Apr 21, 2026
@domlander domlander self-assigned this Apr 21, 2026
@domlander domlander added run_chromatic Runs chromatic when label is applied fronts + curation maintenance Departmental tracking: maintenance work, not a fix or a feature labels Apr 21, 2026
@github-actions github-actions Bot removed the run_chromatic Runs chromatic when label is applied label Apr 21, 2026
@domlander domlander marked this pull request as ready for review April 21, 2026 12:33
@github-actions
Copy link
Copy Markdown

Hello 👋! When you're ready to run Chromatic, please apply the run_chromatic label to this PR.

You will need to reapply the label each time you want to run Chromatic.

Click here to see the Chromatic project.

Copy link
Copy Markdown
Contributor

@SHession SHession left a comment

Choose a reason for hiding this comment

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

Changes make sense, I have tested interactions with the player locally. One small functional change, but otherwise LGTM


const seekBackward = () => {
if (vidRef.current) {
// Allow the user to cycle to the end of the video using the arrow keys
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small change of behaviour here, although it doesn't seem necessarily worth keeping.

Copy link
Copy Markdown
Contributor Author

@domlander domlander Apr 21, 2026

Choose a reason for hiding this comment

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

Ah yes, I did explain the reasoning behind this in the PR to implement a new progress bar, but I forgot to move the comment over when I split this PR out: #15726 (comment)

In short, It's an very minor feature that increase the complexity of the code quite a bit when non-looping (default) video is added, so this is why it's removed.

@domlander domlander merged commit be26afb into main Apr 21, 2026
41 checks passed
@domlander domlander deleted the doml/sh-video-refactor branch April 21, 2026 14:23
@gu-prout
Copy link
Copy Markdown

gu-prout Bot commented Apr 21, 2026

Seen on PROD (merged by @domlander 8 minutes and 15 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fronts + curation maintenance Departmental tracking: maintenance work, not a fix or a feature Seen-on-PROD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants