Use a new interactive progress bar for long-form (default) video#15726
Use a new interactive progress bar for long-form (default) video#15726
Conversation
707eb4a to
4b25f4d
Compare
4b25f4d to
0acd346
Compare
9c31c47 to
574e189
Compare
574e189 to
659c7d3
Compare
| /** | ||
| * Subtitles are anchored to the bottom, but leave enough room for a tall progress bar | ||
| */ | ||
| | 'raised-bottom'; |
There was a problem hiding this comment.
Would love a better name for this! 🙏
There was a problem hiding this comment.
Haha! Claude suggested above-controls?
c843d21 to
4991c1c
Compare
4991c1c to
1051888
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
e9ecb10 to
696a4c1
Compare
696a4c1 to
a4aa151
Compare
SHession
left a comment
There was a problem hiding this comment.
I've tested this locally and it behaves as described. I've left a few non-blocking comments, otherwise LGTM!
| const handleKeyDownProgressBar = ( | ||
| event: React.KeyboardEvent<HTMLInputElement>, | ||
| ): void => { |
There was a problem hiding this comment.
Is there a reason not to have the same key handler as the rest of the video? The controls are mostly the same and I can't think of a reason not to include the additional ones incase the user tries them.
There was a problem hiding this comment.
The initial idea was that we didn't need mute, for example, on the progress bar, and if there were any future keyboard additions to either the progress bar or the video, then it wouldn't inadvertently affect the other. Thinking about it more, you've made a really good point: I can't really think of an example where this would be the case and, as you say, there's no reason not to disallow 'm' to mute the video when focused on the progress bar. I'll combine the two key handlers.
| ); | ||
| }); | ||
|
|
||
| describe('convertProgressPercentageToCurrentTime', () => { |
There was a problem hiding this comment.
It would be nice to have a test case for a 0 or negative duration.
| /** | ||
| * Subtitles are anchored to the bottom, but leave enough room for a tall progress bar | ||
| */ | ||
| | 'raised-bottom'; |
There was a problem hiding this comment.
Haha! Claude suggested above-controls?
| margin-left: 1px; /* To make it _feel_ more aligned with the progress bar, which has a border radius. */ | ||
| `; | ||
|
|
||
| const formatTime = (timeInSeconds: number) => { |
There was a problem hiding this comment.
nit we can add some tests for this as with the time progress percentages.
There was a problem hiding this comment.
Yep, I'll add tests
a4aa151 to
1d2a2a8
Compare
This is better, but the icons are also "controls" and sometimes the controls go to the top of the page, i.e. feature cards. I've gone with |
f38ad48 to
5161156
Compare
| const useLongFormProgressBar = videoStyle === 'Default'; | ||
|
|
||
| const subtitlesPosition: SubtitlesPosition = | ||
| useLongFormProgressBar && controlsPosition === 'bottom' |
There was a problem hiding this comment.
It's a little bit safer to include controlsPosition === 'bottom' in the condition, just in case we decide to use a long form progress bar at the top of the card (i.e. feature cards) in the future
5161156 to
1952d38
Compare
SHession
left a comment
There was a problem hiding this comment.
I've reviewed the code changes and they address the comments 🙏 I've not retested locally, lmk if it would be worth doing.
Thanks for the re-review. I've retested locally on different browsers and still looks good |
|
Overdue on PROD (merged by @domlander 30 minutes and 7 seconds ago) What's gone wrong? |
|
Seen on PROD (merged by @domlander 3 hours, 14 minutes and 49 seconds ago) Please check your changes! |
What does this change?
Implements an alternative progress bar for Default (non-Youtube) video. This progress bar:
Tested on Chrome, Firefox, Safari & Edge; on mobile (using browserstack) and desktop browsers.
I have an alternative implementation of this progress bar which uses
div's with CSS/JS instead of<input type="range" ... />. In this approach, the smooth transition of progress is maintained and it is easier to style and control with CSS/JS. However, scrubbing comes "for free" with theinputapproach, which would be tricky to implement well otherwise, and also has better accessibility by default.Styling sources:
What does this PR not do?
<input type="range" ... />. This is a nice-to-have. Other popular video players on the web have a mixture of smooth and stuttered transitions.Why?
To match designs. Long-form video would benefit more from extra video features.
How to test?
Screenshots
Screen.Recording.2026-04-20.at.15.42.14.mov