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

Revision history for this message
Ian Booth (wallyworld) wrote :

> Hi Ian,
>
> This looks like I nice addition, but I wonder if it's necessary to list the
> branch URLs or if it'd be enough to have just the branch's name (linking to
> the branch itself) and the series/package name (also linking to the relevant
> thing). Something like
>
> v Related branches[1]
>
> ------------------------------------------------------------------------
> | From source packages | Owner |
> ------------------------------------------------------------------------
> | _firefox-4.0_ from the _We're back in the game_ series | TheOwner |
>
> -----------------------------------------------------------------
> | From product series | Owner |
> -----------------------------------------------------------------
> | _bzr-2.2_ from the _Bazaar 2.2_ series | TheOwner |
>
> That makes (what seems to me to be) the important information more prominent,
> avoids the repetition, provides a link to the series/packages themselves and
> allows us to use similar table headings for both tables (having one table list
> the owner and the other just the source package seemed a bit weird to me, but
> maybe there's a reason for doing so?).
>
> Also, do you really need the fixed width on the table? Shouldn't the
> surrounding boxes of the launchpad-form limit the width of the table? Or is
> this because you want them to have the exact same width as the recipe text
> area?
>
> [1] notice the different capitalization, as recommended by
> https://dev.launchpad.net/UserInterfaceWording

Thanks for such a detailed comment - really appreciated. The reason for showing the branch urls is to allow the user to explicitly see what the urls are and so to allow simple copy and paste into the recipe - I believe this to be one of the key drivers for this work; when creating a recipe, it's the most error prone bit and the info is not easily intuited unless it is displayed. The branch name is clear from reading the url hence I didn't see the need to show it.

The reason for showing the owner of the product series branches is that it is not obvious from the url who the owner is. The source package column in the source package branches table is to allow easy navigation to the source package home page. These choice were based on implementation discussions when the work was commenced, however I am more than happy to show whatever information is deemed necessary. If we want to continue showing the urls for the reason mentioned above, it's tricky showing too many columns given the urls are so long and they look terrible when wrapped.

Before I added the fixed table width, the tables expanded out to the right hand edge of the browser window and looked terrible - I'm not sure why the form didn't limit the width. Having the tables a similar width to the other main recipe form widgets (the description text area in particular) seems to look good to me but that's only an opinion.

I'll fix the capitalization.

« Back to merge proposal