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 maxWidth variable that adds wrapper, retains original max width/height #19

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

andyadams
Copy link
Contributor

References issue #10. When the maxWidth option is set to true, an extra container is added that limits the size of the video to the originally-specified dimensions.

My text editor also strips out trailing whitespace, so there's a bunch of that removed as well :).

P.S. Chris, at WordCamp SF I totally ditched mid-talk upstairs to come and hear yours. It was awesome.

@joesavage
Copy link

The maxWidth functionality is really nice, but in the cases where I've tried to use it - it has proved something I've just had to put back to false. This is because stopping the video being 100% width, means that the video isn't centered within it's parent division (and hence looks completely ugly on the page).
I can't really think of any obvious ways to fix this though. Would I be right in saying that it would require some heavier Javascript to center within the parent with a max-width (or am I just being stupid)?

@andyadams
Copy link
Contributor Author

@joesavage,

If you wanted a video to be centered, you could add some styles to make it that way - maybe auto margins? I'm not a CSS master, but it would seem that this would be best handled by simply styling appropriately.

The use case I wrote this for is when someone really wants an embedded video to be a certain width - without my fix, the video would automatically become 100% width and nothing could be floated next to it, for example.

@joesavage
Copy link

@andyadams Unfortunately doing a simple "margin: 0 auto" requires a fixed width (you can't use min or max-width), hence I can't find any easy way to center the video. I agree that the maxWidth addition is very good for floating and stuff - but it's a shame that there is no easy way (that I can see) to center the video, as many people seem to use FitVids.js to put videos in content division (in which you'd want it to be centered).

EDIT: Actually, coming back to this, using automatic margins with a max-width seems to work fine :P

@mhulse
Copy link

mhulse commented Apr 18, 2012

Hi! Love this plugin. Just curious, has there been any movement on this patch?

@andyadams
Copy link
Contributor Author

None from me - @joesavage ?

@dabernathy89
Copy link

This is a great idea. It really should be the default behavior, IMO. It's the same way we treat responsive images, particularly when they are floated with text.

@helgatheviking
Copy link

👍 from me. It'd be nice to not expand the video to larger than its natural dimensions. That ends up looking blurry.

@davatron5000
Copy link
Owner

So I'm here, 2 years later, combing through PRs. 😿

I'd be interested in getting this into (the mythical) FitVids 2.0, but I wonder if could be done with just one setting instead of two:

maxWidth: false // default, do not wrap
maxWidth: true // set max-width to originalWidth
maxWidth: 900 // set max-width to 900px

Thoughts?

@graygilmore
Copy link

I think it only needs to be a single option.

@palewire
Copy link

palewire commented Mar 3, 2014

+1

This was referenced Apr 1, 2014
Thimp and others added 2 commits December 22, 2014 11:37
The current version of the maxWidth setting 'extension' was outdated. This is the updated version with the latest code of FitVids.
Updated maxWidth version of FitVids to 1.1
@pomartel
Copy link

I also think this should be the default behaviour. Other than that, the plugin works very nicely. Great job!

@rchrd2
Copy link

rchrd2 commented Jul 7, 2015

👍 This fork worked for me

fabianfetik pushed a commit to fabianfetik/FitVids.js that referenced this pull request Oct 5, 2015
@shennan
Copy link

shennan commented Nov 16, 2016

@davatron5000 Any movement on this?

@graygilmore
Copy link

@shennan looks like it's being included in the v2 attempt: http://codepen.io/davatron5000/pen/9fef11884a883855d8a1a187bac0cbfe

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.