feat: add media-error-dialog #1024
Conversation
|
|
@luwes 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: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1024 +/- ##
==========================================
- Coverage 78.55% 77.53% -1.03%
==========================================
Files 59 51 -8
Lines 11080 12329 +1249
Branches 0 724 +724
==========================================
+ Hits 8704 9559 +855
- Misses 2376 2742 +366
- Partials 0 28 +28 ☔ View full report in Codecov by Sentry. |
not sure if the video actually pauses when it errors
Co-authored-by: Christian Pillsbury <cjpillsbury@gmail.com>
There was a problem hiding this comment.
lgtm except this should show the error title
Also we're swallowing the custom error code and that doesn't seem right. I think we should instead create a title for unknown errors like "Error Code 42".
In the future we could give devs a way to override the title of a custom code.
src/js/labels/labels.ts
Outdated
| @@ -1,5 +1,14 @@ | |||
| export type LabelOptions = { seekOffset?: number; playbackRate?: number }; | |||
|
|
|||
| // Setting a code explicitely to null makes the error not show up in the UI. | |||
| export const errors = { | |||
| 1: null, | |||
There was a problem hiding this comment.
I'll be interested to see if there's any downsides of swallowing Code 1s completely. No change needed but I wonder if we should log a warning to the dev console.
src/js/media-container.ts
Outdated
| ::slotted([role="menu"]) { | ||
| align-self: end; | ||
| slot[name=dialog]::slotted(*) { | ||
| z-index: 2; |
There was a problem hiding this comment.
I'm not clear on why and I'm sure there's a good reason, but I wish we didn't need this. I've been bitten a lot by unintentional z-index layering.
There was a problem hiding this comment.
There was a problem hiding this comment.
in media-chrome-range the input[type=range] element is used as a hover zone which is 14px higher than the actual media-chrome-range. helps with dragging on mobile. it's primarily for layouts where the timeline goes right above the buttons and then it's better that the timeline slightly overlaps the buttons.
I can see the concerns though, we already had a report from Phil on this in the Mux dashboard with Mux players dialog being on z-index: 100.
I'll see if there is a workaround.
There was a problem hiding this comment.
I did some testing on Mux player with the z-indexes removed and that worked fine because we set them in the themes. maybe that's right approach. leave it up to the consumer of the MC elements?
I removed all the z-indexes in the last commit.
related #1012 requires muxinc/media-chrome#1024 - fixes an issue where the `.error` object could be null when the error event is dispatched. this was introduced when we started calling initialize() function deferred in mux-video I believe. this is fixed by saving the error state each time before we dispatch the error event. - the warning API is not implemented because after closer inspection we don't really do much with non-fatal errors. the only thing is doing a console.warn here: https://github.com/muxinc/elements/blob/1e1f24a18d7fe61c5b4bdfd9cf4cfb6d9d57b0ea/packages/mux-player/src/index.ts#L461-L465 we can easily do that in playback-core if we want?
https://media-chrome-git-fork-luwes-error-dialog-mux.vercel.app/examples/vanilla/control-elements/media-error-dialog.html
use of
<media-chrome-dialog>here:https://media-chrome-git-fork-luwes-error-dialog-mux.vercel.app/examples/vanilla/control-elements/media-settings-menu.html
MediaErrorDialog.formatErrorMessage(error)getTemplateHTMLstatic methods (could be nice to start working towards this)