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.
Hi!
First of all thanks for the module!
As a developer and very often site maintainer, I wanted to suggest to use the Libraries module instead of changing the files within the module directory (wich is ok when you only have 1 or 2 instances to maintain but a pain in the - you know what - when you have more).
I join a patch (I hope it has been correctly made..).
All the best,
Valentin
Comment | File | Size | Author |
---|---|---|---|
#20 | modernizr-fix-libraries-info-999870.patch | 813 bytes | gmclelland |
#16 | 999870-libraries-support-16.patch | 856 bytes | rupl |
#9 | modernizr-libraries.patch | 2.43 KB | theohawse |
modernizr-libraries.patch | 2.45 KB | zeropaper | |
Comments
Comment #1
tamasd CreditAttribution: tamasd commentedI have started working on this feature in the new feature integration branch (7.x-2.x currently, but if there is a demand for that then I can backport it to D7) based on your patch.
I would like to give a proper integration to the libraries module. The problem is that the libraries module wants to include the modernizr.js file directly, but it should not be included directly. The reason is that in order to get the Modernizr triggered, a 'no-js' class should be on the html tag, but in Drupal, there is no way to put classes on the element. So I have created a little helper, which adds the required classes to the html tag and then loads the library.
I have to think about this for a few days, but I try to come up with a smart solution. Meanwhile, I commit the code what I did to CVS.
Comment #2
zeropaper CreditAttribution: zeropaper commentedHopefully, this patch does prevent that behavior ;)
Comment #3
designerbrent CreditAttribution: designerbrent commentedWould love to see this included in the 6.x branch. Libraries is a godsend for those of use that maintain several sites.
Comment #4
zeropaper CreditAttribution: zeropaper commentedI couldn't agree more :) and by the way this patch is made for the 6.x branch. (yorirou, if you do want, I can maintain this branch)
Comment #6
tamasd CreditAttribution: tamasd commentedCan someone verify that the patch works correctly with the D6 version? If so, then I will merge to the -dev version.
Comment #7
ckng+1 for this.
Could just add an instruction to add 'no-js' to html.tpl.php (D7) and page.tpl.php (D6)?
Comment #8
tamasd CreditAttribution: tamasd commentedI wouldn't do it, because my html.tpl.php and page.tpl.php can be overwritten by a different module or theme or modernizr can break a theme or a module, which expects a special html.tpl.php or page.tpl.php.
Comment #9
theohawse CreditAttribution: theohawse commentedThe patch worked great for me, however I noticed they aren't including a .min.js file in the version 1.7 package, but rather just calling it modernizr.js .
So instead of renaming the 1.7 version back to modernizr-1.6.min.js -I've adjusted the patch to call for modernizr.js
here's the commands that worked in the end for the patch above
Will test the patch that I've attached and post results.
Comment #10
theohawse CreditAttribution: theohawse commentedThe new patch worked for me again, should mention this is with the latest dev release of modernizr module.
Here's the commands updated for above
Unless of course I've missed the minified version somewhere in the 1.7 modernizr library. But it really wasn't there.
Comment #11
brunorios1 CreditAttribution: brunorios1 commented+1
Comment #12
klonos...subscribing for a 7.x patch/solution - when available.
Comment #13
gmclelland CreditAttribution: gmclelland commentedInstructions on #10 worked for me in D6. Could we get this added to the dev release?
Comment #14
jantoine CreditAttribution: jantoine commented+1
Comment #15
tamasd CreditAttribution: tamasd commentedPatch merged in commit 081afa72560ed9a7c180af3b068fb915530f182a.
Please test it, and if it works, I'll release the 6.x-1.1 version.
For D7, the 7.x-2.x beta is really close, and that version fixes this problem also.
Thanks for the patience.
Comment #16
ruplI was getting bad calls to the directory "sites/all/libraries/modernizr" instead of the fully qualified path to the file. The attached patch got the requests loading properly. This still seems a bit inflexible because it only looks for modernizr.min.js or modernizr.js
Could we use the regex approach that's present in the 7.x branch to load any permutation that we can usually expect to find?
modernizr.js
modernizr.min.js
modernizr.custom.js
Comment #17
gmclelland CreditAttribution: gmclelland commentedI just tested with 7.x-2.1 and discovered that placing the file in /profiles/nameofprofile/libraries/modernizr/modernizr.min.js doesn't work.
Instructions say to place it in sites/.../libraries/modernizr/modernizr.min.js, but the libraries module should be able to load libraries from within an install profile. The libraries module loads my other libraries in my profile just fine.
Any thoughts on what could be wrong?
Comment #18
gmclelland CreditAttribution: gmclelland commentedNevermind, I ran it again and it worked fine.
Comment #19
ruplOk, cool. I had started looking into this but could not reproduce, glad to hear it's working for you!
Comment #20
gmclelland CreditAttribution: gmclelland commentedIn modernizr.module, I think we need to change this function
I think this should be
In the function above I changed two spots: modernizr_library_info() should be modernizr_libraries_info() and the "title" attribute should be "name".
Patch is attached. The patch should be applied to the 7.x-2.x branch. Hope that helps,
-Glenn
Comment #21
ruplGood catch and thanks, Glenn! Committed to 7.x-3.x
I haven't done it yet, but I will do more clean up on the implementation of this function. Modernizr 2.x doesn't require the
.no-js
class, so it can load directly with a<script>
tag, making the modernizr_loader.js business less of an issue.