Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I use a Responsive Framework like ZURB Foundation 3 on Drupal. This framework use specific CSS div container to make Videos like vimeo or youtube Responsive. http://foundation.zurb.com/docs/elements.php#vidCode
It will be nice to define globally the css class because i need not to define this on every content.
Comment | File | Size | Author |
---|---|---|---|
#28 | css_div_class_around-1927318-28.patch | 389 bytes | malcomio |
#24 | bootstrap_video_container-1927318-24.patch | 2.54 KB | nader@gizra.com |
Comments
Comment #1
deggertsen CreditAttribution: deggertsen commentedHere's a patch for this as I needed this as well so that I could define a max-width attribute for http://drupal.org/project/fitvids. Hope it helps!
Used to fix this issue as well #1874152: Video Filter with max-width
Comment #2
Chris Burge CreditAttribution: Chris Burge commentedWrapping the iframe or object in a div seems common sense. I wrote an identical patch with only a different class name before finding this issue. Patch applies against 7.x-3.1. I don't see any risks with the patch.
Comment #3
Chris Burge CreditAttribution: Chris Burge commentedComment #4
mnlund CreditAttribution: mnlund commentedIt would be a nice additional feature to add a code specific class to the wrapper. This way it is possible to style each wrapper a little differentlly.
Comment #5
Pere OrgaThe patch has been RTBC for more than a year. Please can we have this merged?
Comment #6
DamienMcKennaThis should be added to the next release.
Comment #7
DamienMcKennaShouldn't the attribute be added to the standard field wrapper, rather than adding another DIV?
FYI this will probably need to be rerolled.
Comment #8
malcomio CreditAttribution: malcomio commentedHere's a re-rolled patch.
I don't think it makes sense to add attributes to the field wrapper, as this wouldn't be relevant to the case of video filter in body text.
Comment #9
malcomio CreditAttribution: malcomio commentedI'd also be inclined to include the relevant CSS in the module for responsive videos so that people who aren't using custom themes get the benefit as well.
Should be easy enough for people who do know CSS to override if needed.
Here's a patch that does that.
Comment #10
emmonsaz CreditAttribution: emmonsaz as a volunteer commentedFYI, I had better success with https://github.com/toddmotto/fluidvids
Comment #11
emmonsaz CreditAttribution: emmonsaz as a volunteer commentedHere's my patch draft. It could be improved with a fluidvids module using the Libraries API but for now it works for my needs...
Comment #12
pawel.traczynski CreditAttribution: pawel.traczynski commented@emmonsaz - nice patch, but you should use hook_page_build instead of hook_init to add css or js.
Comment #13
emmonsaz CreditAttribution: emmonsaz as a volunteer commented@pawel.traczynski Nice catch - thanks!
Comment #14
blackdog CreditAttribution: blackdog commentedI will not add a dependency to fluidvids, doesn't make sense to me.
I'm leaning towards committing #8, and let people decide for themselves if and how to use the wrapping div.
Comment #15
DamienMcKennaThis doesn't add a dependency, it just takes advantage if the library is found.
This patch makes the PHP code a little more readable and adds a note to the README.txt about it.
Comment #16
blackdog CreditAttribution: blackdog commentedIs fluidvids the only way to handle this? The best? I'm concerned there will be more if statements for library X & Y & Z if we go down this road. I might be wrong though, so if this is the preferred way, I'm not gonna be stubborn.
Comment #17
DamienMcKennaI don't think the current implementation is hurting anything, if someone else wants to add support for another library then maybe it could be refactored at a later point but I think it's fine for now.
Comment #18
Chris Burge CreditAttribution: Chris Burge commentedAre we abandoning the non-JS approach from #1 - #9 in favor of fluidvids.js? I have #9 implemented on several sites without issue. I have no objection to adding fluidvids.js functionality for those who wish to use it, but if I can solve an issue without JS, that's my preferred route. Take a look at this article, which covers both approaches: http://www.creativebloq.com/netmag/how-add-responsive-video-your-website....
Comment #19
malcomio CreditAttribution: malcomio commentedI'd definitely prefer to avoid adding more JS to the page - IMO it's always best to keep the page weight down and avoid unnecessary JS wherever possible.
Comment #20
Chris Burge CreditAttribution: Chris Burge commentedI don't have an objection to supporting fluidvids.js in addition to #9. The issue that I see us running into is that the CSS from #9 will probably interfere with fluidvids.js
Solution #1
In patch #15, the video_filter_page_build() function checks for the fluidvids.js file before loading it. If the fluidvids.js file doesn't exist, we could load the responsive CSS from #9 using drupal_add_css().
Solution #2
We could also set the wrapper class depending on if the fluidvids.js file exists (e.g. w/o fluidvids.js class="video-filter"; with fluidvids.js class="video-filter-fluidvids"). The CSS from #9 would only apply to the wrapper with the desired class.
Comment #21
blackdog CreditAttribution: blackdog commentedI will be committing the simpler solution (#8), and let the discussion continue.
Comment #22
blackdog CreditAttribution: blackdog commented#8 committed.
Comment #24
nader@gizra.com CreditAttribution: nader@gizra.com at Gizra commentedHere's is a patch to use Bootstrap's responsive videos containers, This should be a good solution for you if you are using bootstrap.
Comment #25
Chris Burge CreditAttribution: Chris Burge commentedBoth the fluidvids.js approach and the Bootstrap approach can be implemented without patching the Video Filter module. The relevant code is contained in theme functions (theme_video_filter_flash and theme_video_filter_iframe), which means it can be overridden. See Overriding theme functions.
Comment #26
DamienMcKennaThis was committed, any further changes can be handled via theming.
Comment #28
malcomio CreditAttribution: malcomio commentedI'm not so sure that this is fixed - as it stands, the wrapper div and the iframe both have a class of 'video-filter', which makes targeting the iframe separately from its wrapper unnecessarily difficult.
Here's a new patch that takes the 'video-filter' class off the video itself.
Comment #29
thalemn CreditAttribution: thalemn commentedInstead of applying the patch, I wrote some javascript to remove the video-filter class if the video-youtube class was present. Maybe useful for others who have the issues I was having with video-filter class in both the container and the iframe.
Nothing fancy but it works: