Improv/styling rework #74
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mux/media-chrome/4rfLfd2B2kVVeRDMNYJ46yXKBGUw |
| color: #ffffff; | ||
| font-family: sans-serif; | ||
| font-size: 14px; | ||
| line-height: 24px; |
There was a problem hiding this comment.
NOTE (opinionated): intentionally dropped line-height as that was causing scaling/sizing issues in various cases for what I believe will be our most common use case, which is using images/svgs for the slots.
There was a problem hiding this comment.
Pay close attention to the layout impacts then, like at the single pixel level. At least the way I CSS, consistent pixel heights is usually dependent on the line height.
examples/basic.html
Outdated
| <media-mute-button></media-mute-button> | ||
| <media-volume-range></media-volume-range> | ||
| </media-control-bar> | ||
| <span slot="custom-chrome"> |
There was a problem hiding this comment.
NOTE (opinionated): Rather than baking in too many assumptions for audio control/chrome layout in media-container, we've added (good idea from Dylan) a "custom-chrome" slot that you can layout however you want. That said, I can see valid arguments for making the "baked in"/"default" layout for audio work like the expected layout of <audio controls/> (while still keeping the custom chrome slot for anyone who wants the escape hatch)
There was a problem hiding this comment.
Yeah, I really don't like that this is in "basic.html", as in it's a required element for basic usage. Media chrome is all about "custom chrome", so needing to add another element to do "custom chrome"... wut?
Let's get specific on what we'd be fixing with this. If this is needed I'd hope it's for the 10-20% of use cases, not 80% of use cases.
There was a problem hiding this comment.
The audio styling/layout behavior was more about deciding whether we want the baked in styling/layout of media-container to be more presumptuous or less presumptuous, since our current architecture doesn't allow us to use the data management parts of media-chrome without also being tied to the UI.
That said, I think your point is a reasonable preference for "default" audio layout behavior. I'll rework so a "media-control-bar only" case "just works" for audio (I think it will also work reasonably well for most of the other plausible audio chromes).
There was a problem hiding this comment.
Yeah, probably not clear, but it's intentionally "media-chrome" not "video-chrome". While we've clearly started with video, audio continuing to be second-class isn't the vision.
I think we're probably on the same page, but I'm also cautious to get super fancy here with too much "just works", since we're still just creating the building blocks for video and audio players.
Just to call out other options I still see as viable:
- Create separate
video-container/controllerandaudio-container/controllerelements, and changing the default layout behavior within those. (basically the same as theaudioattr though) - Be explicit about where the control bar is going. i.e.
<media-controller>
<!-- when building an audio player (debatable if top is best) -->
<media-control-bar slot="top-controls">
<!-- when building a video player... -->
<media-control-bar slot="bottom-controls">
</media-controller>I'd assume in video we know the default slot should go at the bottom and in audio it doesn't matter because the default slot is just going to fill the height of the container by default, so maybe that's reason enough we can make this "just work", without requiring more intentionality from the dev. I'm guessing we're already on the same page there. But we can also already see how different audio themes can get with the Spotify player, and we should be careful of adding too much magic to support that vs. starting to help the dev understand how the layout is working.
since our current architecture doesn't allow us to use the data management parts of media-chrome without also being tied to the UI
Yeah, longer discussion behind that one, but a first step there could be three attrs for the media-controller, audio-layout (replace audio), video-layout (default, at least for now), no-layout.
There was a problem hiding this comment.
Yeah I should be able to make this "just work" for audio (I suspect even for the spotify theme). Just need to add some additional attribute selectors + appropriate CSS under the hood.
src/js/media-container.js
Outdated
|
|
||
| /* Max out at 100% width for smaller screens (< 720px) */ | ||
| max-width: 100%; | ||
| // max-width: 100%; |
There was a problem hiding this comment.
NOTE (opinionated): This should be applied externally, as it's a simple style adjustment, familiar to anyone who may want it, and would be more generically predictable styling/layout behavior for someone unfamiliar with media-chrome.
There was a problem hiding this comment.
I wish I had documented the issue better, but it's solving something... clearly something with smaller screens. An element limited to the width of its container doesn't feel super "unpredictable" to me. Is there more I'm missing?
There was a problem hiding this comment.
Hm. I disagree. That's not really how any html elements behave "out of box". With pretty much any visual HTMLElement, if you don't apply any CSS to them, the child will size itself and the parent will be sized by its children (with different default sizing/overflowing/etc. behavior based on the default display/layout rules of the elements in question).
There was a problem hiding this comment.
Yeah ok, fair enough. We can see if the issue comes up again.
| width: 720px; | ||
| } | ||
|
|
||
| @supports not (aspect-ratio: 1 / 1) { |
There was a problem hiding this comment.
NOTE: Will be upgrading my OS/Safari tonight or tomorrow to (dis)confirm what's shipping rn. If it's v15, I'm with heff and think we should not add complexity for non-evergreen (will also test on available mobile devices).
src/js/media-container.js
Outdated
| <slot name="gestures-overlay"></slot> | ||
| </span> | ||
| <!-- | ||
| <span part="layer text-tracks-layer"> |
There was a problem hiding this comment.
Could uncomment this if we want to anticipate our "let media-chrome optionally manage sub/cc rendering" option (though we'd still need to pin down some of the details before we could fully implement).
| @@ -130,17 +172,15 @@ class MediaContainer extends window.HTMLElement { | |||
| } | |||
|
|
|||
| static get observedAttributes() { | |||
| return ['autohide'].concat(super.observedAttributes || []); | |||
| return ['autohide'].concat(MEDIA_UI_ATTRIBUTE_NAMES); | |||
There was a problem hiding this comment.
NOTE: This is not directly related to styling/layout, at least not in the "core" sense. However, it was (re)added to
- help resolve auto-hide issues for demuxed
- make styling cases easier for theme-like usage
| @@ -172,13 +212,13 @@ class MediaContainer extends window.HTMLElement { | |||
| : MediaUIEvents.MEDIA_PAUSE_REQUEST; | |||
| this.dispatchEvent(new window.CustomEvent(eventName, { composed: true, bubbles: true })); | |||
| } | |||
| media.addEventListener('click', this._mediaClickPlayToggle, false); | |||
| // media.addEventListener('click', this._mediaClickPlayToggle, false); | |||
There was a problem hiding this comment.
note: not styling related in a core sense, but having this behavior "baked in" was causing issues in demuxed use case, and would for many other reference player "chromes" which behave differently than "click means play pause". This could instead be handled by the newly added "gestures" layer, and we can discuss as a followup if we want any "default gestures" like this.
| @@ -12,12 +12,12 @@ import { Window as window, Document as document } from './utils/server-safe-glob | |||
| import { MediaUIEvents, MediaUIAttributes } from './constants.js'; | |||
| import { verbs } from './labels/labels.js'; | |||
|
|
|||
| const enterFullscreenIcon = `<svg aria-hidden="true" xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"> | |||
There was a problem hiding this comment.
NOTE (opinionated): We should allow the viewBox to determine the relative sizing and allow the button's css to determine the computed sizing, as this will allow us to more easily have responsive designs and behaviors. Sizing and resizing can still work if someone has width/height properties set, but it may be more cumbersome and not work as expected in all cases. This is in the end based on the way vector graphics sizing and dimensions work, so providing these recommendations are not especially unique to media-chrome (see, e.g.: https://css-tricks.com/scale-svg/)
There was a problem hiding this comment.
Nothing about that sounds wrong. It'd be helpful to see an example of this working.
| @@ -11,16 +11,34 @@ import { nouns } from './labels/labels.js'; | |||
| const DEFAULT_RATES = [1, 1.25, 1.5, 1.75, 2]; | |||
| const DEFAULT_RATE = 1; | |||
|
|
|||
| const slotTemplate = document.createElement('template'); | |||
There was a problem hiding this comment.
Since we removed some of the font-centric "baked in" stuff from media-chrome-button, this is the current proposed solution for buttons that will use text. (We could also add a "text-button" superclass that media-playback-rate-button extends)
|
Trying to piece together some documentation to understand what this looks like from the DevEx perspective. ControlsOften times for your web player you want two sets of controls, one set for mobile on small screens that have a touch interface and another set for desktop devices that have a mouse interface. For former (mobile) often times you want to overlay the controls on the media player. You can do something like this: SlotsSlots are used to tell the slot="top-controls"Specify an element like slot="controls-overlay"Specify an element like slot="bottom-controls"Specify an element like Media control bar by default will be a control bar pinned to the bottom of the media element <media-controller>
<!-- use the slot="top-controls" to overlay controls at the top of the player -->
<div slot="top-controls">
<media-mute-button></media-mute-button> <!-- put the mute button in the top left -->
<div class="spacer"></div>
<media-pip-button> </media-pip-button> <!-- put the pip and fullscreen buttons in the top right -->
<media-fullscreen-button></media-fullscreen-button>
</div>
<!-- use the slot="controls-overlay" to overlay controls in the middle of the player -->
<div slot="controls-overlay">
<media-seek-backward-button></media-seek-backward-button>
<media-play-button></media-play-button>
<media-seek-forward-button></media-seek-forward-button>
</div>
<!-- use the slot="bottom-controls" to overlay controls at the bottom of the player -->
<div slot="bottom-controls">
</div>
<!-- media-control-bar will positioned at the bottom of the player by default -->
<media-control-bar>
<media-time-range></media-time-range>
<media-time-display></media-time-display show-duration>
</media-control-bar>
</media-controller>Here's some examples
^ Is this roughly what the documentation would look like for using the new "slots"? Bugs, Takeaways, Possible improvements
Similarly, I think I had to do the same thing for
^ This feels like a lot to figure out, could we make this easier? I'm not sure why autohide wasn't automatically working, and I'm also wondering if all overlays should have autohide behavior by default when the media is playing. |
|
Trying to piece together some documentation to understand what this looks like from the DevEx perspective. ControlsOften times for your web player you want two sets of controls, one set for mobile on small screens that have a touch interface and another set for desktop devices that have a mouse interface. For former (mobile) often times you want to overlay the controls on the media player. You can do something like this: SlotsSlots are used to tell the slot="top-controls"Specify an element like slot="controls-overlay"Specify an element like slot="bottom-controls"Specify an element like Media control bar by default will be a control bar pinned to the bottom of the media element <media-controller>
<!-- use the slot="top-controls" to overlay controls at the top of the player -->
<div slot="top-controls">
<media-mute-button></media-mute-button> <!-- put the mute button in the top left -->
<div class="spacer"></div>
<media-pip-button> </media-pip-button> <!-- put the pip and fullscreen buttons in the top right -->
<media-fullscreen-button></media-fullscreen-button>
</div>
<!-- use the slot="controls-overlay" to overlay controls in the middle of the player -->
<div slot="controls-overlay">
<media-seek-backward-button></media-seek-backward-button>
<media-play-button></media-play-button>
<media-seek-forward-button></media-seek-forward-button>
</div>
<!-- use the slot="bottom-controls" to overlay controls at the bottom of the player -->
<div slot="bottom-controls">
</div>
<!-- media-control-bar will positioned at the bottom of the player by default -->
<media-control-bar>
<media-time-range></media-time-range>
<media-time-display></media-time-display show-duration>
</media-control-bar>
</media-controller>Here's some examples
^ Is this roughly what the documentation would look like for using the new "slots"? Bugs, Takeaways, Possible improvements
.overlay {
flex-grow: 1;
display: flex;
align-items: center;
justify-content: center;
}Similarly, I think I had to do the same thing for
media-controller[user-inactive="user-inactive"]:not([media-paused]) :is(.top-controls, .overlay, media-control-bar)) {
opacity: 0.01;
transition: opacity 1s;
}^ This feels like a lot to figure out, could we make this easier? I'm not sure why autohide wasn't automatically working, and I'm also wondering if all overlays should have autohide behavior by default when the media is playing. |
Here's the "core" of the <span part="layer media-layer">
<slot name="media"></slot>
</span>
<span part="layer gesture-layer">
<slot name="gestures-overlay"></slot>
</span>
<!--
<span part="layer text-tracks-layer">
<slot name="text-tracks-renderer"></slot>
</span>
-->
<span part="layer controls-overlay-layer">
<slot name="controls-overlay"></slot>
</span>
<span part="layer controls-layer">
<slot name="top-controls"></slot>
<slot name="middle-controls"><span class="spacer"></span></slot>
<slot></slot>
<slot name="bottom-controls"></slot>
</span>
<media-controller class="media-theme-youtube">
<video
slot="media"
src="https://stream.mux.com/DS00Spx1CV902MCtPj5WknGlR102V5HFkDe/high.mp4"
muted
preload="none"
crossorigin
>
</video>
<media-control-bar slot="bottom-controls">
<!-- controls go here -->
</media-control-bar>
</media-controller>I think your broader point is maybe something like "why do we need a |
src/js/media-container.js
Outdated
| <slot name="text-tracks-renderer"></slot> | ||
| </span> | ||
| --> | ||
| <span part="layer controls-overlay-layer"> |
There was a problem hiding this comment.
What's the difference between controls-overlay-layer and controls-layer, which also has controls that are overlaid on the video?
There was a problem hiding this comment.
In my experience, "overlays" are generally things that cover the entire real estate of the thing they are "overlaying" (so things like an "overlay" for gesture/touch interactions, an "overlay" for controls that can be placed anywhere on top of the player/chrome, etc). The controls layer, on the other hand, is broken up into vertical "regions" with corresponding slots, so more of an "anchored to a region" (even though yes, they are, "overlayed" in the generic sense).
We probably want a single layer for "overlay controls" (aka the ones that look like they're "on top" of the video). I played around with using the "middle-controls" for that originally, but then we end up with weird and/or complicated layout issues if we e.g. show/hide the control bars, , or add a top control bar, or even change the size of the control bar/its controls.
This relates to Dylan's proposal here, which I generally agree with (at least at the high level):
- When using slot="controls-overlay" I was expecting things to be automatically centered, but I had to add some code for that, is this expected?
From what I've seen "in the wild", overlay controls are generally horizontally centered over the chrome/player, regardless of what other things are showing/hiding. Letting them live in their own "layer" makes styling them more predictable and will be clear/clean/predictable/resilient behavior regardless of other things you're adding to your chrome by having them on their own "layer".
I think there are some other wins as well that we can discuss by keeping them on their own "layer", but we can discuss further for sure.
There was a problem hiding this comment.
@dylanjha this also relates to your:
- The naming feels a little inconsistent "top-controls", "controls-overlay", and "bottom-controls" -- technically the top and bottom are "overlays" too (in a sense that they are overlaid onto the media element. top-overlay, center-overlay, bottom-overlay -- would that make more sense?
…n. Update examples to reflect changes.
…ia-container slots.
…ble browser player.
…roof for plausible custom subs/cc renderer used in middle-chrome. Update docs. Remove redundant css.
…rs in media-container/media-controller per PR feedback.
…de and aspect-ratio and include examples. Fix autohide in media-container. Update examples for autohide and for better clarity with slots demo.
…r per PR feedback from heff. Update examples/demos to reflect changes.
… media effects user-inactive behavior.
…slotted elements (change align-items to start instead of stretch, unless media-control-bar).
Hah this was another example of some "presumptuous styling under the hood" instead of "less presumptuous styling under the hood". Before I was always applying |
Ok I've got a "fix" that resolves the issue in the baked in styling in |
cjpillsbury
force-pushed
the
improv/styling-rework
branch
from
b03c650
to
c424470
Compare
Reworking some of the DOM and styling structures to make use cases such as responsive design easier, less code, and more predictable for those less familiar with media-chrome.
Changes include:
media-playback-rate-button