Code review comment for lp:~jcsackett/launchpad/branch-collection-links-652126

Revision history for this message
Paul Hummer (rockstar) wrote :

<rockstar> jcsackett, the code looks fine.
<rockstar> jcsackett, as far as UI though, I think you should put the commit count, branch count, and user count on the same line.
<rockstar> Active reviews should be its own little section.
<jcsackett> rockstar: i tried that, but you end up with either a really weird block of text, or it looks like the branches and users *also* occured in the commit time period.
<jcsackett> rockstar: you think move active reviews back into its own side thing?
<rockstar> jcsackett, X branches, owned by X people. There were X commits in the last X period.
<rockstar> jcsackett, the intention of the X commits in the last X period is to show how active the project is.
<jcsackett> rockstar: yeah, that just looked wierd to me, but i'll deal with the text block instead of dividing it up as it is.
<rockstar> It doesn't do a good job, but I think laying it out that way would be better.
<jcsackett> rockstar: regarding active reviews; side thingy, or just a separate line?
<rockstar> Active reviews need to be obvious. Putting text near it makes it hard to identify.
<rockstar> jcsackett, separate line should be fine.
<jcsackett> rockstar: dig.
<rockstar> jcsackett, for bonus points, it doesn't show if there are now active reviews.
<jcsackett> i'll make those changes.
<jcsackett> rockstar: i'll look into it. didn't want this growing beyond the scope of bridging the gap, but if it's easy to throw in (and it should be) i'll hit it.
<rockstar> jcsackett, yeah. UI reviews often cause feature creep. Truthfully though, I think it stays in line with the original purpose of the bug.
<jcsackett> rockstar: dig.
<jcsackett> i'll switch that up and then ping you when i've made the changes, rockstar.
<rockstar> jcsackett, great, thanks.

« Back to merge proposal