Code review comment for lp:~wallyworld/launchpad/recipe-find-related-branches

Revision history for this message
Tim Penhey (thumper) wrote :

On Tue, 07 Dec 2010 03:11:15 you wrote:
> On 06/12/10 14:22, Tim Penhey wrote:
> > Review: Needs Fixing
> > I have two issues with this branch.
> >
> > 1) What do we show when we can't find any related branches?
>
> Nothing. The page appears as it does now before the branch. Do you want
> some text to say something like "No related branches"? I didn't think
> that was needed but can easily add it.

No, I think that nothing is better. I take it that the fieldset isn't rendered
at all when there are no branches?

> > 2) Creating a widget is overkill for what is needed, which is some text
> > on the page.
>
> The page template uses this construct to render the form:
>
> <div metal:use-macro="context/@@launchpad_form/form" />
>
> The first implementation this branch involved replacing the above with a
> macro:
>
> <div metal:use-macro="context/@@+sourcepackagerecipe-macros/recipe-form"/>

There is a simpler way, which I happen to have done with my "ppa creation
during new recipe" branch, which does expand all the rendered widgets.

The form macro defines a number of slots which can be replaced. However with
my new change (which I know you didn't know about), it is easier to just add a
simple rendering, and have the methods to get the branches on the Add view
class. Perhaps you might want to merge my branch and make yours dependent on
it?

This is one of the problems of having multiple people hacking on the same
area.

« Back to merge proposal