jquery: hiding module package grouping

Changing the title; this isn't AJAX or even AHAH

I'm not sure on the inline vs .js file. I've tried to keep as little js in modules as possible.

However the way it is now really wont work. When I click a checkbox to enable a module it hides the package. Yikes!

I like the idea of using the heading to control the collapsing of the packages. In fact, that's exactly where I was hoping it would go. Two nitpicks, it doesn't look very clickable, and right now its expanding all the packages areas rather than than just its packages. I think the icon idea is probably a good idea. Possibly something like the drupal menu carrots you can see over there. -->

Thanks for getting this going nickl. I'm sure this will make some people happy.

My 2c lies on external JS.

I'm not sure about this part :
$(this).removeClass().addClass("package-heading-collapsed");
Seems like a bad idea to remove the classes alltogether.
Could there be a toggleClass-based solution instead ?

What is the value of this patch? What problem does it fix? I'm not convinced this is a useful change.

Wehn you have 150 modules in 25 tables, it's really helpful to collapse a few of them so...

Yes, this would be _really_ handy.
This is the natural next step after http://drupal.org/node/81740, it would be a shame not to do this.

I even think all packages should be collapsed by default (except maybe core ?), and the open / collapsed states stored in the session (like admin/node filters)

Plus comment on the patch :
currently, all the expand/collapse triggers in core (menu, fieldsets) are links ('a' tag). This should probably be the case here as well (the 'a' tag being JS-inserted, like for fieldset labels)

the patch in #6 has a typo : $ouptut instead of $output (took me a while to find out)

Here's another proposition for this patch.
In fact, I think a simpler and cleaner approach would be to use collapsible fieldsets :
- it's more coherent with the rest of admin forms
- a non-negligible amount of time was spent on polishing the collapsible.js thing (nice, smooth, not-too-short-not-too-long transition, "intelligent" scrolling to keep the content in view...), so it provides a much cleaner effect than what the patch in #6 does ATM.
- the wanted functionality is exactly the same, do we want code duplication ?

Additional UI should probably be added to have "show all / hide all" behaviours.
This patch doesn't tackle the session saved states I mentioned in comment 10 above. Any feedback on the idea ?

PS : I'm aware that the idea of fieldsets was mentioned in http://drupal.org/node/81740 (comments #20 and #21). The argument back then was "fieldsets take too much space". Yet they take less space than current h2's, so I went ahead with this :-)

Screenshot?

Here's a screen shot with all packages collapsed

and another one with devel package expanded

Yeah, the main benefit of this patch is dealing with the size of the module page. We've added a great many useful features to the page, and each one has made the page much longer and somewhat unwieldy.

For example, the descriptions for payment.module and product.module in the ecommerce package take up 5 lines because of the dependency information. Many other modules have at least one line added especially if they are in a package where dependencies are more likely. On top of that, the package is adding its name to the page.

The fieldset(or jquery snippet) addressess this by cutting down on a lot of scrolling for admins.

In responce to the fieldsets, in my previous comments I think I was actually refering to adding more to the patch. I felt it important to not get tied up in javascript enhancements and not get the patch committed. Even so, I didn't think I would like them but they actually feel pretty good. I guess they also have a standard "Drupal Feel" too. They do fullfill the clickable comment I made earlier as well.

Also, the starting collapsed works pretty well as when I've been playing with it the first thing I've been doing is collapsing 90% of the fieldsets.

Good work.

Not sure this is the proper way to implement this. If someone overwrites theme_fieldset(), this page could break, or become inconsistent. Using theme_ functions seems necessary.

Dries : you're probably right.
I'm currently away from my code environment so I won't be able to propose another patch before another 2 weeks. Anyone is welcome to tackle this instead :-)

No, I think Dries meant that the "fieldset" tag should be generated by a call to theme_fieldset, instead of being hardcoded as it currently is.

Not sure this is exactly what you expect, but here is a patch that calls to theme('fieldset'..) from 'theme_system_modules'.

I'm currently unable to test by myself, so I won't set it to RTBC, but the code looks good.

the patch in #24 works as intended, so I set this back to RTBC
Here's a re-roll against current cvs.

Works for me! Thanks.

I think this should not have all the fieldsets collapsed. Right ow with a default install you see two lines:

Core - optional
Core - required

Lets pretend we have never seen this before and are trying to enable some modules. What are you going to click on?

We should make this useful by default and expand one of the fieldsets.

This patch keeps 'Core - optional' fieldset expanded by default.
(In 'Core - required' there is nothing to configure so its useless to expand that.)

New patch looks good. Marking this RTBC as all looks solid.

Correct version.

Also as coding standards mean, a comma is missing at the end of the fieldset array.

Updated patch. We need to compare translated strings (for $package and 'Core - optional'), not raw strings. Also added the extra comma at the end of the array.

Apart from that, looks good, and definitely needed (and tested - works fine). RTBC.

We need to compare translated strings, not raw strings.

Why that, it should also work with both strings untranslated, not?
But I'm also fine with this patch.

it should also work with both strings untranslated, not?

Yeh, it would work with both strings untranslated - but AFAIK, it's better practice to compare translated strings in Drupal code.

imnsho this has resulted in a net loss of usability. see http://drupal.org/node/97342 for rant. :P

It appears Dries committed an earlier patch - the one committed is not #34 (which leaves Optional open by default). With Core - Optional open, it'd certainly lessen webchick's rant (and my agreement with it).

I'd actually advocate:

'#collapsed' =>  ($package_title == t('Core - required')) ? TRUE : FALSE,

That's the only one that you can't do anything about, so makes sense to leave that collapsed by default. All the rest should be expanded by default, so that new users can view what they have in terms of options without having to click 100 times (or however many package groups they have after they install all their contribs), and so you can instantly see what you have available to you in terms of modules.

That said, though, just about anything is better than what we have now.

Ideally, it would remember the last state of the collapsed bit when the page re-loads after submission. However, since the fieldsets are being added in @ the theme layer and not to the actual form itself, I could not figure out a way to do this easily.

Its not too bad to follow #39, I think. Its a solution everyone can live with. But please dont remove the fieldsets completely. For experienced users it is a really helpful improvement IMO.

Here is the latest patch modified to only hide 'Core - required' by default.

I'm all for keeping the fieldsets - every fieldset I make always has collapsible => TRUE on because, yes, it is extremely helpful. I just see no utility in going to a form and seeing no field widgets save for "Submit" ;)

#39 is fine with me. It still let's users collapse fieldsets if they want, but leaves it open for new users to see.

Committed to HEAD with a couple modifications:
- ? TRUE : FALSE is useless, there is no need to say "if true, then true, else false."
- The reason for comparing t()-ed strings is when you don't know the original, such as when it was more common to directly look at $_POST['op']. We know both original strings, so we can use them.

Ahhhhhh... :) Much better, thank you!! :D