Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xiukun.zhou’s picture

Status: Active » Needs review

thanks klonos.
http://users.tpg.com.au/j_birch/plugins/superfish/
drupal 7 jquery default version is 1.4.4 not support superfish 1.7, you can using hook_library_alter().

klonos’s picture

I realize that. The point is to allow users to upgrade to newer versions of the libraries easily without having to write any code. This can be done with #1411268: Support Libraries API for the js files..

Do you think that you can consider creating a new 7.x-3.x branch that will add jQuery Update and Libraries API as requirements (that is common for many modules since D7 core is left so far behind with jQuery). Then completely remove the libraries shipped with the module (besides nice_menus.js I guess) and require people to download and place all required libraries under /sites/all/libraries?

This way, people that want something that simply works out of the box will use the 7.x-2.x branch, while more advanced people that want to upgrade to latest versions can use the 7.x-3.x branch.

Another idea is to do what jQuery Update 7.x currently does. It includes a /replace/jquery directory that in turn has a /1.5 a /1.7 and a /1.8 subdirectories. So, you can also ship the module with multiple versions of the libraries and then simply check for the version of jQuery used and based on that switch to using the respective supported version.

I still believe that no matter what, fixing #1411268: Support Libraries API for the js files. first is the right way to go. This will take the burden of shipping libraries off from you and make it a task of the site builder.

xiukun.zhou’s picture

klonos’s picture

Thanx for understanding and considering this. It's really important to be able to upgrade libraries without having to code something.

xiukun.zhou’s picture

Category: task » feature
Status: Active » Needs review
FileSize
2.09 KB

Hi klonos,

I think this is a very good idea and I have tried to spend some time to try to get to an initial testing version of this feature.
So basically, for now, I have created a simple patch for testing to see if we could detect for different JQuery versions to be supported.

The attached patch:
nice_menus-upgrade-superfish-library-2001616-5.patch

should support:

  • Updated jquery.hoverIntent
  • Updated jquery.bgiframe
  • Updated superfish

Download plugins to:

  • site/all/libraries/jquery.hoverIntent/jquery.hoverIntent.minified.js
  • site/all/libraries/jquery.bgiframe/jquery.bgiframe.min.js
  • site/all/libraries/superfish/superfish.js

we could modify the file names and folders path if necessary later on.

If necessary, we could surely create another branch for that, but with this patch, I have the feeling updated JQuery functions could be supported.

Let's try to do some testing of this patch and we will see how to move forward after some feedback.

I would greatly appreciate to have your feedback, reviews, testing and reporting on this approach and patch/code.

Please let me know if you would have any questions, comments, issues, recommendations, objections or concerns on the attached patch or any of the aspects discussed in this comment, I would certainly be glad to provide more information, explain in more details or re-roll the patch if necessary.

Thanks very much in advance to all for your feedback, reviews, testing and reporting.
Cheers!

xiukun.zhou’s picture

i have updated the patch, Thanks everybody tests the patch

klonos’s picture

Sorry, I simply didn't find time to test this out. I promise I will as soon as I can though.

roymuir’s picture

Just patched this module and downloaded the files into the Library. I can confirm that this indeed does work. It solved my issue of having to double tap Nice Menu drop down menu parents to activate the drop down on iOS. Thanks very much!

deggertsen’s picture

For some reason when I went from 7.x-2.3 to 2.x with the patch in #6 it broke all the javascript on my site. Not even the menus worked right (no fade in/fade out). I'm not sure what details would help, but as soon as I found out it wasn't working I switched back to 2.3 and everything was fine again.

deggertsen’s picture

Ah, I figured out my problem. I had to make sure I had jQuery Update set to at least version 1.7 rather than 1.5.

So with this patch jQuery 1.7+ is necessary. 1.8 is better because then when you touch a drop down it doesn't load the page until you've tapped it a second time.

xiukun.zhou’s picture

Status: Needs review » Active

Thanks deggertsen, klonos and roymuir to help me test the patch

kkuhnen’s picture

Thanks xiukuk.zhou - this patch works for me.
I did initially have the same issue as deggertsen (jQuery update was set to 1.5) but once this was set to 1.7+ everything worked as expected.
Thanks again!

deggertsen’s picture

Status: Active » Reviewed & tested by the community

Should this be marked as RTBC? It is working and has been confirmed so by 4 users. It may not be good to commit simply because it would then create a jQuery Update Dependency, but that's up for debate.

klonos’s picture

Status: Reviewed & tested by the community » Needs work

...I think that in order to avoid the issue of people forgetting to switch to jQuery 1.7 even after installing jQuery Update we should detect the jQuery version and throw some warning with a link to the jQuery Update settings page.

Even better, in order to keep things clean I say we switch to a new 7.x-3.x branch and make jQuery Update a requirement officially. That accompanied with proper documentation in both the README.txt as well as the project's page would suffice.

DrupalGideon’s picture

Personally as a user of this module I would prefer to see a 7.x-3.x release branch with a dependency on jQuery Update module. That makes perfect sense. And the quickest way to get this released.

xiukun.zhou’s picture

Status: Needs work » Active

see: ed1af8e

Download plugins to:
site/all/libraries/jquery.hoverIntent/jquery.hoverIntent.js
site/all/libraries/jquery.bgiframe/jquery.bgiframe.js
site/all/libraries/superfish/superfish.js

klonos’s picture

Title: Upgrade included Superfish library to the latest version 1.7.4 (currently shipping 1.4.8) » Upgrade included Superfish library to 1.7.2 (currently shipping 1.4.8)
klonos’s picture

Title: Upgrade included superfish library to 1.7.2 (currently shipping 1.4.8) » Upgrade included Superfish library to the latest version 1.7.4 (currently shipping 1.4.8)

...1.7.4 is out.

gillarf’s picture

Title: Upgrade included Superfish library to 1.7.2 (currently shipping 1.4.8) » Upgrade included Superfish library to the latest version 1.7.4 (currently shipping 1.4.8)

What should I patch this against? Tried it against the current 7.x-2.x-dev version, and the patch failed:

Hunk #1 FAILED at 165.

***************
*** 165,174 ****
      ),
    );
  
    return $libraries;
  }
  
  /**
   * Implements hook_block_info().
   */
  function nice_menus_block_info() {
--- 165,262 ----
      ),
    );
  
+   $jquery = drupal_get_library('system', 'jquery');
+   if (version_compare($jquery['version'], '1.4', '>')) {
+     $plugins = array(
+       'jquery.hoverIntent' => array(
+         'js' => 'jquery.hoverIntent.minified.js',
+         'version_pattern' => '@hoverIntent\s+(r[0-9]+)@i'
+       ),
+       'jquery.bgiframe' => array(
+         'js' => 'jquery.bgiframe.min.js',
+         'version_pattern' => '@version\s+([0-9a-zA-Z\.-]+)@i'
+       ),
+       'superfish' => array(
+         'js' => 'superfish.js',
+         'version_pattern' => '@Superfish\s(v[0-9\.-]+)@i'
+       )
+     );
+ 
+     // config libraries directory.
+     $searchdir = array(
+       'libraries/',
+       conf_path() . '/libraries/',
+       'profiles/' . drupal_get_profile() . '/libraries/',
+       'sites/all/libraries/'
+     );
+ 
+     foreach ($plugins as $plugin => $file_info) {
+ 
+       $libraries_file = NULL;
+ 
+       // check libraries module.
+       if (module_exists('libraries') && libraries_get_path($plugin) != '') {
+         $library_directory = libraries_get_path($plugin);
+         if (is_file(DRUPAL_ROOT . '/' . $library_directory . $plugin . '/' . $file_info['js'])) {
+           $libraries_file = $library_directory . $plugin . '/' . $file_info['js'];
+         }
+       }
+ 
+       // scan dir.
+       if ($libraries_file === NULL) {
+         foreach ($searchdir as $libraries_dir) {
+           if (is_file(DRUPAL_ROOT . '/' . $libraries_dir . $plugin . '/' . $file_info['js'])) {
+             $libraries_file = $libraries_dir . $plugin . '/' . $file_info['js'];
+             break;
+           }
+         }
+       }
+ 
+       // add to libraries.
+       if ($libraries_file !== NULL) {
+         $libraries[$plugin]['version'] = nice_menus_get_version($libraries_file, array(
+           'pattern' => $file_info['version_pattern']
+         ));
+         $libraries[$plugin]['js'] = array($libraries_file => array());
+       }
+     }
+   }
+ 
    return $libraries;
  }
  
  /**
+  * Gets the version information from an arbitrary library.
+  *
+  * @return
+  *   A string containing the version of the library.
+  */
+ function nice_menus_get_version($file, $options = array()) {
+   // Provide defaults.
+   $options += array(
+     'file' => '',
+     'pattern' => '',
+     'lines' => 10,
+     'cols' => 200,
+   );
+ 
+   $file = DRUPAL_ROOT . '/' . $file;
+ 
+   if (!is_file($file)) {
+     return;
+   }
+   $file = fopen($file, 'r');
+   while ($options['lines'] && $line = fgets($file, $options['cols'])) {
+     if (preg_match($options['pattern'], $line, $version)) {
+       fclose($file);
+       return $version[1];
+     }
+     $options['lines']--;
+   }
+   fclose($file);
+ }
+ 
+ /**
   * Implements hook_block_info().
   */
  function nice_menus_block_info() {
xiukun.zhou’s picture

Hi gillarf:
download 7.x-2.5

gillarf’s picture

Thanks - just tried that, and got the same patch error.

The patch looks straightforward though, I'll try to apply the changes manually and see if that works.

klonos’s picture

...this is now fixed in 7.x-3.x. The library does not mention any minimum/maximum jQuery requirements, so I guess that it could be packported to 7.x-2.x. It does recommend hoverIntent r7 though and that in turn requires jQuery 1.9.1+. I currently have no interest to keep using 2.x, so if somebody tests latest Superfish in 7.x-2.x with hoverIntent r7 without using jquery_update and they report it works, please feel free to reopen this issue and request backport.