Media Controller Refactor #517
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
@@ Coverage Diff @@
## main #517 +/- ##
==========================================
+ Coverage 75.50% 76.52% +1.01%
==========================================
Files 41 43 +2
Lines 7009 7045 +36
==========================================
+ Hits 5292 5391 +99
+ Misses 1717 1654 -63
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
src/js/media-controller.js
Outdated
| get: function(controller){ | ||
| const { media } = controller; | ||
|
|
||
| return (media) ? media.paused : true; |
There was a problem hiding this comment.
nit (non-b): unneeded parentheses
|
LGTM so far 👍 |
|
At a decent merge point now, with other 1.0 work looming. Followup steps:
Also Codecov really wants more tests. I don't think that should be a blocker to merging this since there wasn't added functionality. |
| @@ -629,36 +836,33 @@ class MediaController extends MediaContainer { | |||
| mediaSetCallback(media) { | |||
| super.mediaSetCallback(media); | |||
|
|
|||
| // TODO: What does this do? At least add comment, maybe move to media-container | |||
There was a problem hiding this comment.
@gkatsev what does this do? (I'll add a comment)
There was a problem hiding this comment.
This makes the media element focusable if a user didn't specify a tabindex already. -1 means that it'll be focusable via a mouse click or programmatically, but not be part of the tab order.
I believe it's necessary to keep focus inside the player for things like keyboard shortcuts.
| propagateMediaState(this.mediaStateReceivers, stateName, state); | ||
| // TODO: I don't think we want these events to bubble? Video element states don't. (heff) |
There was a problem hiding this comment.
@cjpillsbury confirming it's ok to turn off bubbling here. I don't have a strong defense but it feels wrong to bubble these state change events
There was a problem hiding this comment.
I believe I added this to account for nested component use cases so we wouldn't need to know if some other bit of code happened to add an event handler on e.g. a theme, or a wrapping component, etc (similar to considerations for composed: true). Since I can't easily confirm if anything would break and this would be a breaking change, I'd recommend keeping it for this iteration of the refactor and we can determine if we can remove it for simplicity during the v1.0 effort.
src/js/utils/utils.js
Outdated
| * @param {number} ms | ||
| * @return {Promise} | ||
| */ | ||
| export const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); |
There was a problem hiding this comment.
nitpick(blocking): missing EOF newline
|
Aside from us banging on this for smoke testing around the edges (particularly in non-desktop & streaming cases), this looks great as an initial step to create some architecture seams! |
src/js/media-controller.js
Outdated
| static get observedAttributes() { | ||
| return super.observedAttributes.concat('nohotkeys', 'hotkeys', 'default-stream-type'); | ||
| } | ||
| const MediaUIStates = { |
There was a problem hiding this comment.
One thing that we're losing in the Delegates->MediaUIStates transition is the concept of a default delegate. A lot of the these just need to do a property or attribute reference on the media element, and now they all need to be included explicitly.
src/js/media-controller.js
Outdated
| static get observedAttributes() { | ||
| return super.observedAttributes.concat('nohotkeys', 'hotkeys', 'default-stream-type'); | ||
| } | ||
| const MediaUIStates = { |
There was a problem hiding this comment.
I think it would be good to move this into a separate file. It'll help clean up the diff for this file (media-controller) as well.
src/js/media-controller.js
Outdated
| // TODO: Returns undefined and a string, consider a better type | ||
| // not tied to attribute strings |
There was a problem hiding this comment.
a couple of questions/thoughts (non-blocking):
- should this return an empty string for the empty case?
- instead of a string, should this be a tuple (an array in the meantime) that contains the start and end?
[start, end], then consumers can stringify as they want. The default case would be an empty array. - in addition, should this account for multiple seekable ranges?
src/js/media-controller.js
Outdated
| }, | ||
| MEDIA_CAPTIONS_LIST: { | ||
| get: function(controller) { | ||
| // TODO: Move to non attr specific values |
There was a problem hiding this comment.
Should be fine to leave as is for now. These would get updated when we combine captions and subtitles lists.
|
Looks like there are conflicts now. Hopefully, they're easy to resolve. |
| } from './utils/captions.js'; | ||
|
|
||
| let volumeSupported; | ||
| export const volumeSupportPromise = hasVolumeSupportAsync().then((supported) => { |
There was a problem hiding this comment.
NOTE: Currently exporting this based on our status quo impl of checking/propagating volume support in MediaController.
No functional changes, just reorganizing and moving things out of the file to make it easier to split out controller logic from the custom element.
The commits are intentional steps in the process (so far).