Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Adding "fluid" mode #1440

wants to merge 1 commit into from

Conversation

albell
Copy link
Contributor

@albell albell commented Aug 21, 2014

This is a first pass at the ideas discussed in #982.

This is a first pass at the ideas discussed in #982.
@heff
Copy link
Member

heff commented Aug 21, 2014

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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...

Copy link
Member

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.

@heff
Copy link
Member

heff commented Aug 22, 2014

This is a great start. Thanks Alex.

@heff
Copy link
Member

heff commented Mar 30, 2015

Closing in favor of #1952. Thanks for getting this started @baloneysandwiches.

@heff heff closed this Mar 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants