-
Notifications
You must be signed in to change notification settings - Fork 7.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding "fluid" mode #1440
Adding "fluid" mode #1440
Conversation
This is a first pass at the ideas discussed in #982.
That's great, thanks! Can't review it right at the moment but will get to it soon. |
.vjs-fluid { | ||
-moz-box-sizing: content-box; | ||
-webkit-box-sizing: content-box; | ||
box-sizing: content-box; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this mess up the control bar when applied since other css might not be expecting content-box
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, content-box
is the universal browser default. So everything in the CSS has always written with that assumption. These lines are just a guard in case of authors/frameworks using the star selector and resetting the box model globally at the top of the stylesheet, which would break responsive percentage padding. Lots of folks do use the star selector fix a la Paul Irish, e.g. Bootstrap. This isn't a change so much as insurance that things stay the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, I misunderstood what was happening.
Frankly, we should just move to borderbox wholesale as it will make all the other stuff a lot nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, moving to border-box is a good note for 5.0. #1444
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the content-box definitions are valuable and not different from the default then they should probably go on the .video-js class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought is just to leave content-box
specific to vjs-fluid
for now, because it will make #1444 easier. Or we can switch it out, and then back again later? Either way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can do that.
This is a great start. Thanks Alex. |
Closing in favor of #1952. Thanks for getting this started @baloneysandwiches. |
This is a first pass at the ideas discussed in #982.