feat(packages): add time display toggle#1669
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📦 Bundle Size Report🎨 @videojs/html
Presets (7)
Media (10)
Players (5)
Skins (30)
UI Components (39)
Sizes are marginal over the root entry point. ⚛️ @videojs/react
Presets (7)
Media (9)
Skins (27)
UI Components (33)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (14)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (10)
📦 @videojs/spf — no changesEntries (4)
ℹ️ How to interpretJS sizes are initial static graph totals (minified + brotli). Lazy dynamic chunks are shown separately when present.
Run |
sampotts
commented
Jun 9, 2026
Comment on lines
+43
to
+46
| const TOGGLE_LABELS: Partial<Record<TimeType, string>> = { | ||
| current: 'Current time, press to show remaining time', | ||
| remaining: 'Remaining time, press to show current time', | ||
| }; |
Collaborator
Author
There was a problem hiding this comment.
Will need to update i18n PRs when this is merged.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
sampotts
commented
Jun 9, 2026
sampotts
commented
Jun 9, 2026
| <div class="${time.controls}"> | ||
| <media-time-group class="${time.group}"> | ||
| <media-time type="current" class="${time.current}"></media-time> | ||
| <media-time toggle type="current" class="${time.current}"></media-time> |
Collaborator
Author
There was a problem hiding this comment.
I only added it to the minimal skin as the default skin felt a bit better with the remaining in place of the duration. We'll revisit this before GA though I guess as we may want to make minimal a bit more minimal in terms of UI. I'm just waiting for the menus stuff to merge so we're reasonably feature stable for the skins.
sampotts
force-pushed
the
feat/time-inversion
branch
from
7e84c02 to
30ff9cc
Compare
sampotts
force-pushed
the
feat/time-inversion
branch
from
30ff9cc to
fdcbd0a
Compare
sampotts
force-pushed
the
feat/time-inversion
branch
from
fdcbd0a to
77ee050
Compare
sampotts
force-pushed
the
feat/time-inversion
branch
from
77ee050 to
4040e28
Compare
sampotts
force-pushed
the
feat/time-inversion
branch
from
4040e28 to
205a982
Compare
sampotts
force-pushed
the
feat/time-inversion
branch
from
205a982 to
436267d
Compare
sampotts
force-pushed
the
feat/time-inversion
branch
from
436267d to
255dbe0
Compare
mihar-22
approved these changes
Jun 30, 2026
mihar-22
left a comment
mihar-22
left a comment
Member
There was a problem hiding this comment.
Thanks for doing this! A second pass and simplifications on additions to Time components would be great, plus considering removing the toggle labels from core.
Comment on lines
+43
to
+47
| const TOGGLE_LABELS: Record<TimeType, string> = { | ||
| current: 'Current time, press to toggle display', | ||
| duration: 'Duration, press to toggle display', | ||
| remaining: 'Remaining time, press to toggle display', | ||
| }; |
Member
There was a problem hiding this comment.
I assume this would complicate the i18n work. I think the ARIA attributes are already indicating that this can be pressed/toggled. YT seems to only require the addition of role and tabIndex. Ideally we can remove.
Collaborator
Author
There was a problem hiding this comment.
I've asked in the shared a11y about the best pattern here. I don't think YouTube has this quite right as there's no indication of what the button actually does, the label only informs of the current state. I could be wrong but waiting on clarification. I think we want the current state and next state to be obvious to screen readers.
Comment on lines
+38
to
+45
| applyElementProps( | ||
| this, | ||
| { | ||
| onClick: this.#handleClick, | ||
| onKeyDown: this.#handleKeyDown, | ||
| }, | ||
| { signal: this.#disconnect.signal } | ||
| ); |
Member
There was a problem hiding this comment.
For perf reasons it'd be great if these event listeners are attached when toggle is true.
Comment on lines
+100
to
+114
| get #activeType(): TimeType { | ||
| return this.#canToggle ? this.#displayType : this.type; | ||
| } | ||
|
|
||
| get #canToggle(): boolean { | ||
| return this.toggle; | ||
| } | ||
|
|
||
| #getMedia() { | ||
| try { | ||
| return this.#state.value; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
I think we can remove all of this?
| }; | ||
|
|
||
| #handleKeyDown = (event: KeyboardEvent): void => { | ||
| if (event.target !== event.currentTarget || event.defaultPrevented) return; |
Collaborator
Author
There was a problem hiding this comment.
The element can have children that could also fire events but we can remove it. Checking that default is prevented also allows folks to prevent the toggle but perhaps we don't need that either.
| } | ||
|
|
||
| #handleClick = (event: MouseEvent): void => { | ||
| if (event.defaultPrevented || !this.#canToggle || !this.#getMedia()) return; |
Collaborator
Author
There was a problem hiding this comment.
Checking that default is prevented allows folks to prevent the toggle but perhaps we don't need that. The rest can go if we're only binding when toggle is enabled.
| }; | ||
|
|
||
| const handleKeyDown = (event: KeyboardEvent<HTMLTimeElement>) => { | ||
| if (event.defaultPrevented || (event.key !== 'Enter' && event.key !== ' ')) return; |
Member
There was a problem hiding this comment.
Same comment as HTML, we might have a utility for key press/click, if not great to create one!
|
|
||
| const handleKeyDown = (event: KeyboardEvent<HTMLTimeElement>) => { | ||
| if (event.defaultPrevented || (event.key !== 'Enter' && event.key !== ' ')) return; | ||
| event.preventDefault(); |
Collaborator
Author
There was a problem hiding this comment.
As above - space will scroll the page.
| ); | ||
|
|
||
| const toggleType = () => { | ||
| if (!canToggle) return; |
Member
There was a problem hiding this comment.
Not guarding anything meaningful. In addition, won't be called because listeners below won't be attached unless it can toggle.
Comment on lines
+101
to
+108
| function getNextType(type: TimeType, defaultType: TimeType): TimeType { | ||
| if (defaultType === 'current') { | ||
| return type === 'remaining' ? 'current' : 'remaining'; | ||
| } | ||
|
|
||
| return type === 'duration' ? 'remaining' : 'duration'; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Only called in one place, fine to inline for now imo.
| toggleType(); | ||
| }; | ||
|
|
||
| const toggleProps = canToggle ? { onClick: handleClick, onKeyDown: handleKeyDown } : undefined; |
Member
There was a problem hiding this comment.
I think it'd be fine to inline this but leave it up to you.
sampotts
force-pushed
the
feat/time-inversion
branch
from
255dbe0 to
6f691de
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6f691de. Configure here.
sampotts
force-pushed
the
feat/time-inversion
branch
from
6f691de to
ee8b10f
Compare
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Slightly different to the issue in that:
type="duration" toggleswitches duration ↔ remaining.type="remaining" togglestill switches remaining ↔ duration.type="current" togglestill switches current ↔ remaining.Why? I noticed Apple Music toggles between duration and remaining and that pattern also worked well with the default skin. Minimal's time layout suited the standard toggle between current and remaining. Happy to run through it on a call if need be.
Validation
Closes #1422
Note
Low Risk
Player UI and accessibility labeling changes with broad test coverage; no auth, data, or playback pipeline changes.
Overview
Adds a
toggleprop/attribute so time displays can switch between paired modes:current↔remaining, orduration↔remaining(initial view still comes fromtype).TimeCoregains toggle-aware labels and attrs: default accessibility text is the humanphrase(not static role names),aria-valuetextis removed, and when toggle is on the element getsrole="button",tabIndex, and anaria-labelthat includes the next action (e.g. “Show remaining time.”).TimeElementandTime.Valuetrack an active display type, handle click/Enter/Space, respectpreventDefault, and reset whentypeortogglechanges or media state is missing.Built-in audio/video skins wire this up: minimal skins toggle current time; default skins use
toggle type="remaining"on the end time slot (duration ↔ remaining). Skin CSS adds focus-visible styling for toggleable times. Docs and tests cover core, HTML, and React behavior.Reviewed by Cursor Bugbot for commit ee8b10f. Bugbot is set up for automated code reviews on this repo. Configure here.