launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Thanks for the great review.

>
> #282/4
> I'd make the module name just lp.bugs.duplicate(s). Think of it more like a python module. You have a class in there MarkBugDuplicate, and this module might contain other code later so no sense pegging it tight.
>

Agreed.

> #299
> Can we move all the rendering logic over to a render() method? The initializer is really for setting things up, and if it calls this.render() that's cool, but it seems like a lot of work for the initializer to do.
>
> Probably something like:
> https://pastebin.canonical.com/70793/
>

I've changed to use the standard renderUI and bindUI methods. Of course,
when the widget is instantiated, the caller now needs to invoke
widget.render() which I've done in bugtask_index.

My reason for not using this approach originally is that the widget
relied on pre-existing markup on the page and I didn't see the loading
of the form as a render operation.


> #354
> Can we prefix methods that aren't meant as part of the public api with _ as we would in Python code? 
>

Yes.

> #390
> Could perhaps then be setup to use the _bind_events methods from above?
>

Since I used bindUI, there's no need to call this explicitly.

> #415
> indent the code in the method
>

Doh.

> #477
> Instead of finding the first node to insert before just use prepend on the container
> http://yuilibrary.com/yui/docs/api/classes/Node.html#method_prepend
>
> Finally, some of the code has comments with yui doc notes. Can you update the others with at least he basics for method and @private. I'm hoping at some point to start to generate a docs build so that we can see what code we have with basic api to the modules.
>

Done.

-- 
https://code.launchpad.net/~wallyworld/launchpad/private-dupe-bug-warning-943497/+merge/116248
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.