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

Comments

Yorirou’s picture

Version:6.x-1.x-dev» 7.x-2.x-dev
Status:Needs review» Active

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

zeropaper’s picture

Version:6.x-1.x-dev» 7.x-2.x-dev

The problem is that the libraries module wants to include the modernizr.js file directly

Hopefully, this patch does prevent that behavior ;)

designerbrent’s picture

Would love to see this included in the 6.x branch. Libraries is a godsend for those of use that maintain several sites.

zeropaper’s picture

I 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)

Yorirou’s picture

Version:7.x-2.x-dev» 6.x-1.x-dev
Status:Active» Needs review

Can someone verify that the patch works correctly with the D6 version? If so, then I will merge to the -dev version.

ckng’s picture

Version:7.x-2.x-dev» 6.x-1.x-dev
Status:Active» Needs review

+1 for this.

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.

Could just add an instruction to add 'no-js' to html.tpl.php (D7) and page.tpl.php (D6)?

Yorirou’s picture

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

theohawse’s picture

Status:Needs review» Needs work
StatusFileSize
new2.43 KB

The 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

cd sites/all/modules/modernizr
wget http://drupal.org/files/issues/modernizr-libraries.patch
patch < modernizr-libraries.patch
rm modernizr-libraries.patch
cd ../../libraries
wget https://github.com/Modernizr/Modernizr/zipball/master --no-check-certificate
unzip master
rm master
mv Modernizr-Modernizr-ff9562a/ modernizr-1.6.min.js

Will test the patch that I've attached and post results.

theohawse’s picture

Status:Needs work» Reviewed & tested by the community

The 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

cd sites/all/modules/modernizr
wget http://drupal.org/files/issues/modernizr-libraries_0.patch
patch < modernizr-libraries_0.patch
rm modernizr-libraries_0.patch
cd ../../libraries
wget https://github.com/Modernizr/Modernizr/zipball/master --no-check-certificate
unzip master
rm master
mv Modernizr-Modernizr-ff9562a/ modernizr

Unless of course I've missed the minified version somewhere in the 1.7 modernizr library. But it really wasn't there.

brunorios1’s picture

+1

klonos’s picture

...subscribing for a 7.x patch/solution - when available.

gmclelland’s picture

Instructions on #10 worked for me in D6. Could we get this added to the dev release?

jantoine’s picture

+1

Yorirou’s picture

Status:Reviewed & tested by the community» Needs review

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

rupl’s picture

StatusFileSize
new856 bytes

I 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

gmclelland’s picture

I 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?

gmclelland’s picture

Nevermind, I ran it again and it worked fine.

rupl’s picture

Ok, cool. I had started looking into this but could not reproduce, glad to hear it's working for you!

gmclelland’s picture

StatusFileSize
new813 bytes

In modernizr.module, I think we need to change this function

function modernizr_library_info() {
  $libraries = array();

  $libraries['modernizr'] = array(
    'title' => t('Modernizr'),
    'vendor url' => 'http://modernizr.com',
    'download url' => 'http://modernizr.com',
    'version arguments' => array(
      'file' => 'modernizr.min.js',
      'pattern' => MODERNIZR_VERSION_REGEX,
    ),
    'files' => array(
      'js' => array(
        // This array is intentionally left blank.
      ),
    ),
    'integraton files' => array(
      'modernizr' => array(
        'js' => array(
          drupal_get_path('module', 'modernizr') . '/modernizr_loader.js',
        ),
      ),
    ),
  );

  return $libraries;
}

I think this should be

function modernizr_libraries_info() {
  $libraries = array();

  $libraries['modernizr'] = array(
    'name' => t('Modernizr'),
    'vendor url' => 'http://modernizr.com',
    'download url' => 'http://modernizr.com',
    'version arguments' => array(
      'file' => 'modernizr.min.js',
      'pattern' => MODERNIZR_VERSION_REGEX,
    ),
    'files' => array(
      'js' => array(
        // This array is intentionally left blank.
      ),
    ),
    'integraton files' => array(
      'modernizr' => array(
        'js' => array(
          drupal_get_path('module', 'modernizr') . '/modernizr_loader.js',
        ),
      ),
    ),
  );

  return $libraries;
}

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

rupl’s picture

Version:6.x-1.x-dev» 7.x-3.x-dev
Status:Needs review» Fixed

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

Status:Fixed» Closed (fixed)
Issue tags:-Libraries

Automatically closed -- issue fixed for 2 weeks with no activity.