Hello,

After installing comment notify on a website using the colorbox module, all my colorbox effects on images click are not working.

Regarding the js error console I found that :

Error : $("#edit-notify-type input", context)[0] is undefined
Source file : example.com/sites/all/modules/comment_notify/comment_notify.js
Ligne : 9

Disabling comment notify, my colorboxes work again.

Please help :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

First, it would be great if you could try #1221870: Javascript attachment problem when comment form is rendered via Panels which changes the javascript a bit.

What are your "Available subscription modes" on admin/config/people/comment_notify ?

gagoo’s picture

OK,
Thanks for your answer.
I tested the patch you advised to me without solving my issue.

But I think I found the solution.
This javascript try to access to the "#edit-notify-type input" element of the DOM.
But if you choose, in the module options, to allow only one type of subscription (eg. subscribe to all comments), this form element will not be added to the DOM however the js file still try to access to it.
So it causes a js error, and I don't know why this error makes colorbox not working.

So I think it should be great if the "comment_notify_form_comment_form_alter" function (in comment_notify.module) would test if the "#edit-notify-type input" element will be added to the DOM before including the "comment_notify.js" file at line 96.

For now I just commented this line, that solved my issue but it is OK only for my configuration.

What do you think about it ?

Thanks

gagoo’s picture

greggles’s picture

Title: Comment notify makes Colorbox module not working » Only add javascript if the settings mean it will be helpful

Sure, that makes sense to me.

j0rd’s picture

Same problem.

Javascript assumes #edit-notify-type is enabled, but I do not allow my users to change the type in the module settings so this element does not exist.

I added a test in the module to see if it exists before trying to do anything.

Needs to be resolved.

greggles’s picture

@j0rd, Could you provide that as a patch?

j0rd’s picture

See attached.

I'm not 100% sure if this will help for all cases, but it removes the error for me.

sun’s picture

Status: Active » Needs review
FileSize
1.1 KB

There you go ;)

neclimdul’s picture

Looks good. It changes the behaviour slightly when #input-notify-type is not checked by still modifying the checkbox in #edit-notify-type but since its hidden it shouldn't matter. Much tighter and cleaner code.

This is only a visual review since I don't have a good place to test this at the moment.

moonray’s picture

Status: Needs review » Reviewed & tested by the community

Works for me as well.

greggles’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
808 bytes

Ok, now that part is fixed, thanks sun, neclimdul, and moonray. http://drupalcode.org/project/comment_notify.git/commit/a0cea69

Now, to remove the JS unless it's fully necessary see attached patch - back to needs review for that.

sun’s picture

I wouldn't recommend to do that.

Rather, the opposite. Move this stuff into hook_init() and unconditionally load it by passing the "every_page" flag to (at least) drupal_add_css().

The JS is minimal lines of code now, the CSS probably too. So the overall aggregation impact will be much larger if this gets unconditionally aggregated and loaded in the default aggregate, which means that we save two separate HTTP requests on almost all pages where comment_notify appears (and since it mostly appears on nodes, that can be counted as >= 90%).

j0rd’s picture

I agree with sun on this one. It's much better from a performance standpoint to integrate all javascript every time. Conditional JS includes cause re-downloads of JS that's already been downloaded with aggregation and are performance death.

greggles’s picture

@sun, @j0rd, I think I agree with you in part and disagree in part. The js is only meaningful if the sitewide settings for multiple notification types are enabled. So if they are disabled we can leave the JS out and it will never be added. I agree with your perspective about adding it to hook_init and passing the every_page flag.

sun’s picture

Status: Needs review » Needs work
+++ b/comment_notify.module
@@ -54,6 +54,18 @@ by creating your own user with a few clicks here [site:login-url]
+  // Add CSS/JS. We only need the JS if they have options.

1) I do not understand this comment.

2) It would be beneficial to document the underlying reasoning for why this is being done.

+++ b/comment_notify.module
@@ -54,6 +54,18 @@ by creating your own user with a few clicks here [site:login-url]
+  drupal_add_css(drupal_get_path('module', 'comment_notify') . '/comment_notify.css', $options);
...
+    drupal_add_js(drupal_get_path('module', 'comment_notify') . '/comment_notify.js', $options);

Use a single $path for the drupal_get_path() result?

+++ b/comment_notify.module
@@ -91,10 +103,6 @@ function comment_notify_form_comment_form_alter(&$form, &$form_state, $form_id)
   // Add the checkbox for anonymous users.
   if ($user->uid == 0) {

Is there an additional $uid == 0 condition to cater for?

Anonymous and authenticated users usually get separate aggregates anyway, so a corresponding condition is valid even with the every_page option.

greggles’s picture

Status: Needs work » Needs review
FileSize
1.42 KB

I updated the comment and changed to a $path variable - good idea.

There is not an additional uid == 0 condition to cater for from the perspective of the css/js, at least not until #417584: make the "available subscription modes" a permission becomes part of this and that seems somewhat far off.

sun’s picture

+++ b/comment_notify.module
@@ -54,6 +54,21 @@ by creating your own user with a few clicks here [site:login-url]
+function comment_notify_init() {
+  // Add CSS/JS.
+  $options = array('every_page' => TRUE);

I'd still recommend you to document in code why you're adding CSS/JS on all pages instead of only when needed, as I'm fairly sure that others will have the exact same idea like you had yourself earlier (and that's good). But unlike many other sloppy modules, this is done on purpose, and the meaning of every_page may not be clear to everyone, and even if it was, the ultimate reasoning would still be unknown. But of course, that's not my beer, I can only recommend... ;)

greggles’s picture

Title: Only add javascript if the settings mean it will be helpful » Only add javascript if the settings mean it will be helpful, optimize for sitewide performance
Status: Needs review » Fixed

I see now. I wasn't clear what you wanted additional comments to detail.

Committed - http://drupalcode.org/project/comment_notify.git/commit/1d22e7a

Thanks for the reviews sun, and code, everyone.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

more appropriate url instead of spam domain