Code review comment for lp:~thumper/launchpad/edit-commit-msg-link

Hi Tim,

As we discussed on IRC, your approach is good. However, I do have a question or two about how you chose to integrate the widget into Launchpad.

I'm marking this as Needs Information for now. If you have already considered and confirmed the answers, then feel free to land the code with r=mars.

First, I don't understand why you had to create your own wrapper for the multiline editor. Did the inline editor's activator widget not work? The Activator is used to make the current set of inline editors flash and spin.

Line by line:

• 13: I strongly suggest using a different namespace to store the static reference to the widget. The 'lp' namespace would be an acceptable place.
• 27: Is the message variable reliably an empty string, or could it possibly be null? Check the inline editor's message attribute getter. Does the inline editor have an isEmpty() method?
• 50: Do you want to use e.preventDefault(), or e.halt()?
• 56: Too bad you have to call a protected method to trigger the editor. That is a wart in the API.
• 89: See my above comment about using the 'lp' namespace to host the global variable instead of using Y itself.

var lpns = Y.namespace('lp');
if (!lpns.widgets) { lpns.widgets = {}; }
lpns.widgets['%s'] = widget;

Maris