search

nkoehler / mat-video

:tv: mat-video is an Angular 8/9+ video player using Material!
https://nkoehler.github.io/mat-video/
MIT License
91 stars 45 forks source link

Responsive Video #1

Closed mediapack-me closed 6 years ago

mediapack-me commented 6 years ago

PLEASE FILL THIS OUT IF YOU WANT HELP WITH AN ISSUE!

First of all, excellent job! Already migrated to the 6s. Impressive.

Onto the task at hand.
This may be more of a feature request, but given the current landscape of the web/mobile, responsiveness is a core expectation.

Feature request

The reqeust is to allow '%' values as a parameter for video height and width.
Material is responsive at it's core.
My sense is that a Material-Video Component should be responsive as well.

What is the current behavior?

The video sizing is rigid and fixed accepting a number. Because 100 isn't a percentage, it gives you a 100 pixel wide video.

What is the expected behavior?

Ideally, the parameters would accept % as well as a number. Or maybe just have an isResponsive option.

What are the steps to reproduce?

Providing a StackBlitz reproduction is the best way to share your issue.
StackBlitz starter: https://goo.gl/wwnhMV
Adding a % to the height or width parameter will cause the compile to fail (as it should).

What is the use-case or motivation for changing an existing behavior?

There are many ways to manage component sizes and responsiveness. We could do calculations based on a stream from the BreakpointObserver for instance, but it has been my experience that a more fluid (%) based approach works best for responsive tasks.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Angular 6 / Material 6, TypeScript 2.7.2, all

Is there anything else I should know?

Again... BRILLIANT Work.

After looking at the code for a bit, I think you can deliver a fully responsive video by just using the same style settings as you use for Full Screen.

If #mat-video# sets the internal video width to 100%. And the #mat-video# video is contained within a flex or other responsive parent element, it should fill the width of the parent element the way a normal html5 video does if you set it's width to 100%. Then the height or aspect ratio should take care of itself.

I just love your progress bar treatment and the overall clean look of this! I can't wait to use it!

Thank you

K

nkoehler commented 6 years ago

Thank you for the feedback! I appreciate the comments, and you're right, material should be responsive. You've given me a good lead because theoretically I just need to update the mat-video container and everything else should fall into place. I am currently investigating.

mediapack-me commented 6 years ago

Thank you for responding so quickly. Your #mat-video# is a fantastic project. It's amazing how tight the progress and buffering work and now well they integrate visually into a material app. The Angular team is really starting to hit their stride this last year. Now that they have a more mature product, I have a feeling your #mat-video# is going to be a big hit.

Thank you

K

mediapack-me commented 6 years ago

We have this working.

Here are the steps:

// if (this.width || this.height) {
//     const res: VideoSize = this.calculateAspectRatioFit(this.videoWidth, this.videoHeight, this.width, this.height);
//     const style = {
//         width: `${res.width}px`,
//         height: `${res.height}px`,
//     };
//     return style;
// } else {
//     const style = {
//         width: `${this.videoWidth}px`,
//         height: `${this.videoHeight}px`,
//     };
//     return style;
// }

In all cases, fullscreen or otherwise, return the same const, 

const style = {
                width: '100%',
                height: '100%'
            };

So, you could actually remove the whole getVideoPlayerStyle() function and replace it with a class or inline style. That woud be simpler, less maintenance, less code, less bandwidth. ]

Remember Angular 6 has the concept of an element.
Those are intended to be composable in in non-angular apps. So, if someone was using bootstrap or foundation, they could just drop it in and its responisve quality would just fit in nicely in whatever container they chose. ;-)

You did such a great job with the progressbar,etc.
So, everything else still works as expected and still looks the same.

I hope this helps.

Thanks

K

mediapack-me commented 6 years ago

One more thing. I hate to say this, but the name #mat-video# might be stepping on the material team's toes... The mat- designation is the one they use for their internal material controls. Anyway.

nkoehler commented 6 years ago

Thank you for taking the time to write an implementation. I had something I was working on for this, but I'm going to test your version out as well and use the best code/css from both.

Just a question, with your implementation, were you still able to style it using pixel values for width and height?

nkoehler commented 6 years ago

Your changes are good, I've taken most of it and merged it with my work. I've decided to allow it to be responsive by default as suggested. CSS can be used for any sizing needed which is a lot simpler and leaves control up to the user. With ngClass/ngStyle/CSS, it gives a lot more flexibility, and also simplifies the api. A new release should be available today with the changes.

Thank you for requesting a great feature and working through it with me!

mediapack-me commented 6 years ago

No worries.
Again, we're looking forward to using your clean take on a video interface, so we're actually helping ourselves too. Right? We also know what it feels like to create something which is why we sent suggested instructions rather than a pull request. This is your baby. We understand. ;-) Please feel free to use what you can however you think is best.

Anyway, here's where you are: You CAN use Pixels or % to specify width, but don't do that at the video component layer.
Leave the video component layer fluid. It's lighter, more flexible, and much less prone to error.

You should NOT specify height. The video already knows how tall it is. ;-)
(ASIDE: You could offer height as another way to constrain the size if you wanted to, but I'm not sure that makes much sense in the real world. Videos have an aspect ratio, so height and width are mutually exclusive from our perspective... In my humble opinion, adding height is kind of like shooting yourself in the foot. The person looking to constrain by height is unlikely to be the kind of customer you want. So, they're more likely to complain rather than end up as a happy customer. ;-)

In your sample, you can still use width - so it won't look or feel much different to your end users.
But what you want to do is use the width the way a developing composer using your component would...

You can't know what they'll need, so you need to set it up so it will handle whatever they need. The best way to do that is to let them drop the #mat-video# in a container that fits the width they need. Let the video height take care of itself. If they have a need for more hieght or black bars, they can make the container taller with a black background, and center the #mat-video vertically in the container... Right? What you've done then is solved a complex problem for them without constraining their vision. That's a good component.

(Background that you don't need to read, but: We tried using breakpoint observer first so we could just drop the non-responsive control in. It worked, but it wasn't good. Material's Breakpoints don't go down small phone sizes. So if you use pixels to control the video width, you either end up with a video that doesn't fill the screen, or one that's too big. If you use percentages, and let the aspect ratio take care of itself, the end result is a smooth fluid experience. When the user resizes , the experience is what you'd expect or hope for smooth continuous available space filling behavior. END OF BACKGROUND.)

IMPORTANT PART IS HERE: So, here's what you do:

  1. In the video.component.ts 

    <video  #video class="video" 
        width="100%" // IMPORTANT!
        [src]="src" 
        [attr.autoplay]="autoplay ? true : null" 
        [preload]="preload ? 'auto' : 'metadata'"
        [attr.poster]="poster ? poster : null"
        style="width:100%; height:auto !important;">
    This browser does not support HTML5 video.
</video>

    What makes this work is adding width="100%" so the HTML5 video is "thinking in percentages"

  2. Create a container in your sample.component.html

    <div // your sidenav content area>
<div class="content video" style="height:auto !important;" [style.width]="width">
  <mat-video [src]="src" 
                      [title]="title" 
                      [autoplay]="autoplay" 
                      [preload]="preload" 
                      [fullscreen]="fullscreen"
                      [download]="download" 
                      [color]="color" 
                      [spinner]="spinner" 
                      [poster]="poster"
></mat-video>
</div>
</div>

    Notice that the container accepts your width just the same way you were doing with the video before. This way, you're not getting in the way of the HTML5 video control's native behavior. But, #mat-video# does NOT need a width. You already told it to take up 100% of available width, so that's what it will do.

That's all it takes.

IMPORTANT. You DO need to delete this

/* 
.video {
    position: absolute;
    left: 0px;
    top: 0px;
    width: 100%;
    height: 100%;
    z-index: 0;
} */

Or at least delete the absolute positioning. It breaks the container metaphor, and the video ends up ????

Again, I hope this helps.

We're looking forward to using #mat-video# with that sweet progress bar. ;-)

Thanks

K

mediapack-me commented 6 years ago

Again, no worries.

We must have responded at pretty much the same time. Your response actually ended up in earlier than my LONG WINDED one. ;-)

All good.

When it's released, we'll drop it in and let you know how it's working.

Thank You

K

nkoehler commented 6 years ago

Version 2.0.0 has been released with responsive support. The release removes the width and height attributes, causing it to be a major release.

Please let me know how it is working for you. I will leave the issue open for now.

Thanks.

mediapack-me commented 6 years ago

Not sure what's wrong, but npm install didn't work as expected.

It took a LONG time to install.

It threw up a lot of warning and error messages.

In the end, I still had version 1.4 instead of the expected 2.0.

I'm uninstalling, clearing the cache, and retrying.

I'll let you know how it turns out.

Thanks

K

On May 8, 2018 at 7:38 PM nkoehler notifications@github.com wrote:

Version 2.0.0 has been released with responsive support. The release removes the width and height attributes, causing it to be a major release.

Please let me know how it is working for you. I will leave the issue open for now.

Thanks.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/nkoehler/mat-video/issues/1#issuecomment-387575277 , or mute the thread https://github.com/notifications/unsubscribe-auth/Ajb_mn2Vy2LAc2Jn4tQtVLwdFbQhpwZrks5twixpgaJpZM4T1iKS .
nkoehler commented 6 years ago

Working for me on my test environment so it must be an npm issue for you. Hope you resolve it soon!

mediapack-me commented 6 years ago

First, thanks for the shout on NPM. ;-)

That was very nice of you.

We're thrilled here to see it.

On the issue we were having, it was npm.

They added it in this morning.

It's cleaner and lighter and seems to be working fine.

Now that we have a responsive version, we'll integrate (and test) and let you know if we across any issues over the next couple of days.

Thank you

K

On May 8, 2018 at 9:03 PM nkoehler notifications@github.com wrote:

Working for me on my test environment so it must be an npm issue for you. Hope you resolve it soon!

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/nkoehler/mat-video/issues/1#issuecomment-387588323 , or mute the thread https://github.com/notifications/unsubscribe-auth/Ajb_mi0-pRxHOLSJI_Rr3osviIAbE298ks5twkB-gaJpZM4T1iKS .
nkoehler commented 6 years ago

Since there hasn't been any major concerns with this, I am closing this issue.

mediapack-me commented 6 years ago

I would have suggested closing this one as well.