Code review comment for lp:~wallyworld/launchpad/show-ajax-notifications

> >> + remove_notifications('error');
> >> + remove_notifications('warning');
> >> + remove_notifications('informational');
> >> + remove_notifications('debug');
> >
> > I am a bit worried that this might accidentally remove more than wanted.
> Class
> > names like "error" and "warning" seem so general that somebody might use
> them
> > locally. How about "lp-error", "lp-warning" etc. ?
> >
> >
>
> These css class names are already defined in the tales macro. However,
> they are actually "error message", "warning message" etc. The code works
> as is since the matching infrastructure looks for "error.*" etc. There's
> issues with constructing a jquery selector for class names with spaces
> that I haven't worked out yet. I meant to do this before the mp was
> submitted but forgot. Thanks for pointing this out. I'll fix it.

Thanks for the fix. I still think that the names are pretty generic but I guess that is a general problem and we should define a global "lp-" prefix. I just read about YUI's support for automatic class prefixes.

>
>
> >> + // The place where we want to insert the notification divs.
> >> + var last_message = Y.one('div.context-publication');
> >> + var display_notification = function(notification, div_class) {
> >> + var node = Y.Node.create("<div class='"+div_class+"'/>");
> >> + node.set('innerHTML', notification);
> >> + last_message.insert(node, 'after');
> >> + last_message = node;
> >> + };
> The debug, warning, info, error messages are displayed one under the
> other inside the "context-publication" div.

Sorry, I had not looked at what the "publication" diff is. But to correct you, they are displayed *under* the publication. That is obviously the right place for them, so all is fine here.

> > I wonder if all these repetitions could not be placed in loops in some way.
> > That would also make it easier to add more levels, if needed.
> >
>
> Perhaps. It's fairly easy to understand as is and setting stuff up so it
> can be done in a loop will maybe add more boilerplate around it all.
> I'll see what can be done.

Ah, I see you introduced notification_info. That is a good pattern and you could have followed that from the start. ;-) Have a look at this branch to see what I have in mind, although I have not run the code yet.
lp:~henninge/launchpad/wallyworld-show-ajax-notifications-refactor

I don't insist on you using this but it would be more consistent.

That's all. Thank you for your patience. I really learned from this review.

Henning