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.
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.
Comment | File | Size | Author |
---|---|---|---|
#13 | flag_vertical_tabs_followup.patch | 657 bytes | quicksketch |
#9 | flag_vertical_tabs.patch | 1.54 KB | quicksketch |
#5 | 625814-flag-verticaltabs-D6-2.patch | 1.33 KB | Dave Reid |
#4 | 625814-flag-verticaltabs-D6-2.patch | 1.33 KB | Dave Reid |
flag-veritcaltabs-D6-2.patch | 1.38 KB | Dave Reid | |
Comments
Comment #1
quicksketchSounds 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.
Comment #2
Dave ReidYes, 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.
Comment #3
Dave ReidYeah I think the only change would be this would use $form['#attached']['js'][] = ... instead of drupal_add_js(...).
Comment #4
Dave ReidSo 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!
Comment #5
Dave ReidMinor change in the attached JS key to match the D7 library name.
Comment #6
Dave ReidOh, 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.
Comment #7
Dave ReidWorth noting that the flag summary JS was removed as of http://drupal.org/cvs?commit=299738.
Comment #8
Dave ReidAdding tag...
Comment #9
quicksketchI 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. :-(
Comment #10
Dave ReidHeh, no worries. I just felt bad because it got to the point where I had to remove it from vertical_tabs.module. :)
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.
Comment #11
quicksketchThanks 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?
Comment #12
Dave ReidThere isn't a standard on it yet. I came across this problem with pathauto needing to override the path.module fieldset summary:
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.
Comment #13
quicksketchAh, 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.
Comment #14
Dave ReidFYI #654894: Difficult to override a vertical tabs summary JS is the issue for Drupal core.