javascript code is broken (and should use behaviors)

As a starting point, here's the patch we're using in Drupal Gardens.

This works as patched in practice and should become D7 compatible once committed. I believe it is better to commit incremental improvements and then fix it when subscription modes are being worked on.

Ok well:

- Drupal.jsEnabled was removed in Drupal 7, so the current JS just does nothing (it is wrapped in an if (Drupal.jsEnabled), so will never run: http://drupal.org/update/modules/6/7#no-jsenabled
- $(document).ready(function() {}) was never correct to add event handlers in Drupal 6 even, let alone Drupal 7, the new code uses the proper behavior code with the attach method
- the new JS code hides the comment notifiation type radio buttons which have no meaning if you did not select to be notified => usability improvement
- the new code binds to the change event of the checkbox vs. the clicked event => better for accessibility, keyboard navigation
- rather than working backwards updating the checkbox if you selected a radio option for notification, this helps you by picking the first radio as the default if you checked the checkbox (no reason to show the radio options until you choose to be notified as discussed above)
- finally, it triggers the change event, so it runs at the start to properly set the UI for existing notifications
- + it also wraps the code in function ($) which is a Drupal 7 requirement to support more libraries

All-in-all I think the new JS is much better in terms of Drupal 7 compatibility (behaviors, $ wrapping, etc), usability (due to how it displays minimal UI at first and then folds out from there), accessibility (due to how it supports non-mouse interaction) and even code cleanliness. No repeated copy-paste code for just one selector change like in the previous code.

It is clear to me that this is way better compared to what is there. Also, the new patched JS runs on all Drupal Gardens sites today where users choose to use comment notify. The old JS would never do anything due to the first line already (and for its other issues mentioned above).

Helpful review?

Shouldn't this be using FAPI JS states instead of custom JS too?

@Dave Reid: how would you keep the existing behavior with #states? I can see how to replicate the new JS proposal easily with states, but not the old one. Admittedly, I'm a beginner with #states still however, so input welcome.

I think I made two versions for the existing and for the new behavior suggested in the patch to now use #states. Untested though.

BTW I think Drupal 7 has the "-new" approach as a standard. If you have a checkbox that leads to other options, the other options will not show up until you check the checkbox. Even the #states examples are full of such samples. So I think the new approach is better and more in line with how Drupal 7 user interface works generally. I asked Jeff to come and provide his feedback. I'd be surprised if he says we should go against the general pattern (that the new JS patch above implemented and the new states approach implements).

Let's get the states patches reviewed and tested then.

I can't recall the prior recommendation, but I recall the use case being a little more complex? Regardless, if I'm understanding Gabor correctly, and if that's representative of all use cases, what he's proposing makes perfect sense to me. In fact, I can't imagine supporting the idea of a child radio driving the selection of a parent checkbox.

Hrm, well I can't really speak for what I said awhile ago. I just can't remember that far back. But I can't see myself making such a decision unless either I was missing the full context then (or perhaps I'm missing it now).

Sorry for the conflicting opinions, but based on what I'm seeing/hearing now, what Gabor presents feels like the right solution. That is, there's no reason to ask people if they want to be notified on all comments, or just this comment, until they've decided they want to be notified period.

Patches in #8 did nothing about visibility, so if it affected visibility, that should be a bug in the states system I think (which Dave Reid above suggested us to use, instead of ksenzee's patch that I happily explained and RTBC-ed before :). Anyway, I'm glad this got in, yay!