I'm using Panels and this is my HTML output in the page source:

<div class="form-item form-type-checkbox form-item-notify">
 <input type="checkbox" id="edit-notify--3" name="notify" value="1" class="form-checkbox" />  <label class="option" for="edit-notify--3">Notify me when new comments are posted </label>
</div>
<div id="edit-notify-type--3" class="form-radios"><div class="form-item form-type-radio form-item-notify-type">
 <input type="radio" id="edit-notify-type-1--3" name="notify_type" value="1" class="form-radio" />  <label class="option" for="edit-notify-type-1--3">All comments </label>
</div>
<div class="form-item form-type-radio form-item-notify-type">
 <input type="radio" id="edit-notify-type-2--3" name="notify_type" value="2" class="form-radio" />  <label class="option" for="edit-notify-type-2--3">Replies to my comment </label>
</div>

For whatever mysterious reason the #ids are modified so the JS is not attached. In theory it is also possible to output more than one comment form on the page via Panels, so as far as I know the use of #ids is generally discouraged. Using id=* instead of #id works for me:

(function ($) {
  Drupal.behaviors.commentNotify = {
    attach: function (context) {
      $('div[id*=edit-notify-type]', context).hide();
      $('input[id*=edit-notify]', context).bind('change', function() {
        if ($(this).attr('checked')) {
          $('div[id*=edit-notify-type]', context).show();
          if ($('div[id*=edit-notify-type input:checked]', context).length == 0) {
            $('div[id*=edit-notify-type input]', context)[0].checked = 'checked';
          }
        }
        else {
          $('div[id*=edit-notify-type]', context).hide();
        }
      });
      $('input[id*=edit-notify]', context).trigger('change');
    }
  }
})(jQuery);
CommentFileSizeAuthor
#12 1221870-rewrite-of-js.patch1.63 KBmarcoka
#12 comment_notify.js_.txt1018 bytesmarcoka
#8 js_rewrite-1221870.patch1.82 KBmarcoka
#1 1221870_js_reduce_use_of_id.patch1.26 KBgreggles
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Active » Needs review
FileSize
1221870_js_reduce_use_of_id.patch1.26 KB

Got it - thanks. I've attached that js as a patch to allow others to more easily review it.

I wonder if we should move away from those being IDs in the first place since, as you point out, it's possible that they are not truly unique on a page.

kargal’s picture

kargal CreditAttribution: kargal commented

Drupal 7.4
Comment notify 7.x-1.0-beta1
Advanced Forum 7.x-2.0-alpha2

Patch didn't work for me.

After applying it, i replaced line 8 by the following :
if ($('div[id*=edit-notify-type input]', context).length && $('div[id*=edit-notify-type input:checked]', context).length == 0) {

Seems to work fine like this.

(Now that i look at my code, it doesn't look very optimized. I don't have time to improve it. I'm sure somebody will do it :P )

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Needs review » Needs work

This needs to be re-worked after #1224444: Only add javascript if the settings mean it will be helpful, optimize for sitewide performance was created/proposed.

marcoka’s picture

marcoka CreditAttribution: marcoka commented

tested it. patch applyed successfully and it works. in my case the ids changed because i put the comment form into a block.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

@e-anima, can you post an updated patch?

marcoka’s picture

marcoka CreditAttribution: marcoka commented

ok, looked around for a minute. after patch applied somehow the admin_menu is not working anymore and it has somehow messed up the textformat infos at the bottom of the comment form.

marcoka’s picture

marcoka CreditAttribution: marcoka commented

even the first patch unhides all textfilter options that are hidden without it.

i am working on it and will post if i find the solution.

marcoka’s picture

marcoka CreditAttribution: marcoka commented
FileSize
js_rewrite-1221870.patch1.82 KB

i rewrote the whole thing because i did not see the sense in the encapsulated ifs.
it could be optimized because the code inside the change is the same as outside but:

$('input[id^=edit-notify]', context).trigger('change'); doe snot work to fire the change on load.

bryancasler’s picture

bryancasler CreditAttribution: bryancasler commented

I just downloaded the dev version of the module and comment_notify.js looks nothing like the sections that are removed in #8. What version of the module should I be applying this patch to?

marcoka’s picture

marcoka CreditAttribution: marcoka commented

apply it to beta1

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

e-anima - it would be ideal to provide the patch against 7.x-1.x-dev. Otherwise I really am unlikely to figure it out and forward port it.

marcoka’s picture

marcoka CreditAttribution: marcoka commented
FileSize
comment_notify.js_.txt1018 bytes
1221870-rewrite-of-js.patch1.63 KB

ok tested dev version.
this code works, has some comments on it and is easy to understand and read.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

1) If you can't rely on the ID, then use a certain unique CSS class. But the id*= selector is a no-go.

2) Latest patch in #12 violates a ton of coding standards.

OddJob’s picture

OddJob CreditAttribution: OddJob commented

Any chance of an updated patch that complies with coding standards?

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented

I'm not sure what to do with this. It looks like the patch in #12 will still apply, the javascript looks the same. But it relies on id^= selectors. I'm not sure which coding standards this patch is violating. Any help? I'm guessing if *= is out, ^= is probably out too - how could we refactor that piece?