Currently there is flag.module support from inside vertical_tabs.module. We should move it out and into the actual flag.module for the following reasons:
1. This is the way that the D7-version of vertical tabs will work. Sub-modules will do their own support for vertical tabs.
2. It allows the integrating module maintainers to control their own code incase their form items change, etc instead of relying on a patch to a module not their own (ironically, not the case here, but still).
3. Saves unnecessary JS from being loaded when modules are not installed or used.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Sounds good to me. This works with (or is similar to) Drupal 7's vertical tabs implementation too right? It'll be great to get a head-start on the D7 port with some compatible code. Thanks for sending the patch this way, it'll be a good enhancement and definitely belongs in Flag sooner or later.

Dave Reid’s picture

Yes, I believe this is at least very similar to the D7-style integration, just obviously without the module_exists() check. I'm working on an Integration handbook page for Vertical tabs for both D6 and D7.

Dave Reid’s picture

Yeah I think the only change would be this would use $form['#attached']['js'][] = ... instead of drupal_add_js(...).

Dave Reid’s picture

So even cooler news is we now support the #attached => js => array(...) in the vertical tabs backport. I'm also going to be adding #group property support to fieldsets to show which fieldsets should be tabified by default. So now with this updated patch there is nothing that needs to be changed when you port to Drupal 7! HOORAY!

Dave Reid’s picture

Minor change in the attached JS key to match the D7 library name.

Dave Reid’s picture

Oh, and I lied. In the D7 port the actual JS will change a little bit. I'm working to see if I can backport those changes as well.

Dave Reid’s picture

Worth noting that the flag summary JS was removed as of http://drupal.org/cvs?commit=299738.

Dave Reid’s picture

Issue tags: +vertical tabs

Adding tag...

quicksketch’s picture

Status: Needs review » Fixed
FileSize
1.54 KB

I had to revise the patch slightly because it didn't make sense to add another flag.js file to flag (we already have one in theme/flag.js). Instead I added the code to flag-admin.js and pulled it in with the #attached property same as before. Committed to 2.x. Thanks Dave, sorry this took so long. :-(

Dave Reid’s picture

Heh, no worries. I just felt bad because it got to the point where I had to remove it from vertical_tabs.module. :)

+++ flag.module	8 May 2010 20:44:35 -0000
@@ -384,6 +384,13 @@
+          'js' => array(
+            drupal_get_path('module', 'flag') . '/theme/flag-admin.js',
+          ),
+        ),

The only thing I have to add is I highly recommend adding a key of 'vertical-tabs' to the JS file in here. That way in-case another module needs to change the summary JS, they can easily swap it out (very edge case, but you never know).

Powered by Dreditor.

quicksketch’s picture

Thanks Dave Reid, I did modify that line because I wanted to follow the pattern set and used by Drupal 7 core. I don't see how using a key of "vertical-tabs" would help things though, wouldn't you want to key it as "flag-vertical-tabs" or something like that? I didn't even realize you could use arbitrary keys in #attached arrays, is there some kind of recommendation or standard on this?

Dave Reid’s picture

There isn't a standard on it yet. I came across this problem with pathauto needing to override the path.module fieldset summary:

function pathauto_form_alter(&$form, $form_state, $form_id) {
  // Process only node forms.
  if (isset($form['type']) && isset($form['#node']) && $form['type']['#value'] .'_node_form' == $form_id) {
      ...

      // Override path.module's vertical tabs summary.
      $form['path']['#attached']['js']['vertical-tabs'] = drupal_get_path('module', 'pathauto') . '/pathauto.js';

I filed a patch for D7 but it hasn't had much traction and I haven't put much effort to push it either. This problem is going to be worse in D7 because in D6 we only support the #attached from the backport module. In D7 there could easily be more than one JS in a fieldset's #attached. If you have better ideas, I'd love to hear them in the core issue.

quicksketch’s picture

Ah, okay at least I can see what you're intending here. I was thinking we needed to namespace "vertical-tabs", but in fact it's explicitly stating "this is the required JavaScript for vertical tabs for this fieldset". It makes sense to me now. I've reverted the key change to match your original patch.

Dave Reid’s picture

Status: Fixed » Closed (fixed)
Issue tags: -vertical tabs

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