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

On Sat, 28 Nov 2009 04:54:18 Māris Fogels wrote:
> 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.

I'm sorry. I don't understand this at all. What activator widget?

Do you mean this code?

> var widget = Y.lp.widgets['edit-commit-message'];
> widget.editor.on('save', function() {
> commit_message_listener(this.get('value'), true);
> });
> widget.editor.on('cancel', function() {
> commit_message_listener(this.get('value'), false);
> });

If I try to remove the ".editor" it doesn't work. So I can't just attach to
the "save" or "cancel" button of the widget - which is what I'd like to do.

> 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.

Done.

> • 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?

The widget will never give us null, only empty strings.

Since the widget doesn't allow listening on the save or cancel events, they
had to be listened to on the ieditor - and as such the widget is not easily
available to listen to. No the inline editor does not have an isEmpty method.

> • 50: Do you want to use
> e.preventDefault(), or e.halt()?

I don't know. Halt probably, but I was copying from other code. Should we
use halt if we have handled the problem?

> • 56: Too bad you have to call a protected method to trigger the editor.
> That is a wart in the API.

Yep.

Tim