Code review comment for lp:~james-w/launchpad-work-items-tracker/workitem-list-pages

Revision history for this message
James Westby (james-w) wrote :

> Hi James,
>
> I think this looks good but the new functions/methods would benefit from
> some docstrings.

I will add some, thanks.

> Why 3 update() calls instead of just one passing a dict combining
> everything?

No reason really, I'll change it.

> > + $(".workitems_in_status").tablesorter({
> > + sortList: [[1,1],[0,0],[2,0]],
>
> Maybe a comment explaining these magic numbers?

Good idea.

> To make this even nicer you could add something like a
> valid_states_with_labels to report_tools (by zip()ping a list containing
> the lables with the existing valid_states there). :)

I'll do that. I need to see if we can import directly, because
passing it in the data dict would be a bit of a hassle.

Thanks,

James

« Back to merge proposal