Feat/default duration #779
Conversation
…n. Can remove before merge if preferred.
|
@cjpillsbury is attempting to deploy a commit to the Mux Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #779 +/- ##
==========================================
- Coverage 78.67% 78.65% -0.03%
==========================================
Files 58 58
Lines 10627 10644 +17
==========================================
+ Hits 8361 8372 +11
- Misses 2266 2272 +6 ☔ View full report in Codecov by Sentry. |
| @@ -31,13 +31,14 @@ | |||
| <body> | |||
| <main> | |||
| <h1>Media Chrome Advanced Video Usage Example</h1> | |||
| <media-controller defaultsubtitles> | |||
| <media-controller defaultsubtitles defaultduration="134"> | |||
There was a problem hiding this comment.
NOTE: we can revert this if preferred, though I think it might be a good example for advanced.
src/js/controller.js
Outdated
| } | ||
|
|
||
| // Apply defaultDuration if it's set and the media is not yet ready | ||
| if (!(media.readyState || Number.isNaN(defaultDuration))) { |
There was a problem hiding this comment.
why not rely on media.duration? media.duration >= 0 and not NaN
could backfire with custom media elements that have a limited media API
There was a problem hiding this comment.
Interesting thought! I think readyState better represents the actual conditions, but only relying on duration sounds like it would be good simply because it reduces the barrier of entry for custom slotted media elements' API. technically 0 duration media is valid, but I think we can ignore that wild scenario unless/until it comes up. Sold.
There was a problem hiding this comment.
right 0 would be wild :) was thinking current time
…eadyState per PR feedback and discussion.
| (!media || | ||
| !media.duration || | ||
| Number.isNaN(media.duration) || | ||
| !Number.isFinite(media.duration)) |
There was a problem hiding this comment.
If the source is live (Infinite), would this screw up some things?
src/js/controller.js
Outdated
| } | ||
|
|
||
| // If `defaultduration` is unset, we still want to propagate `NaN` | ||
| // for some cases to ensure appropriate media state receiver updates. | ||
| // TODO: What if duration is infinity/live? (heff) |
There was a problem hiding this comment.
Oh, ha. Still a future us problem? Or can we remove this note?
There was a problem hiding this comment.
Didn't wanna be presumptuous, but we can probably remove it. We expect to strip live infinity in our architecture rn and don't have a story for live + our components that (typically) expect a (finite) duration. I'll drop it before merging.
Adds support for
defaultdurationto apply before media is loaded.NOTE: The
<media-time-range>can still be interacted with before the media has loaded, but that was already true.