Launchpad itself

Code review comment for lp:~wgrant/launchpad/bug-540819-fix-builder-list-icons

bug-540819-fix-builder-list-icons
Merge into devel

Revision history for this message

William Grant (wgrant) wrote on 2010-03-29:

20:55 < wgrant> jtv: The 'building' icon spacing on https://edge.launchpad.net/builders seems a bit broken. Is that related to your changes?
20:56 < jtv> wgrant: yes, it is. How appropriate it is depends a bit on your perspective: it's the build status, so I'm displaying it as part of the link to the build. It did simplify the code.
20:57 < jtv> wgrant: in fact this is the subject matter of a UI review that's currently somewhere in limbo.
20:57 < wgrant> jtv: The current spacing is definitely broken. I think the ordering is too, but that's up for debate.
20:58 < jtv> wgrant: I don't think I changed the ordering... did I?
20:58 < wgrant> It was '( ) Building _blah blah blah i386_'
20:58 < wgrant> It is now 'Building ( )_blah blah blah i386_'
20:59 < jtv> wgrant: oh, sorry, misunderstood you earlier. That's the part I was talking about all along.
20:59 < wgrant> jtv: Why does it make the code simpler?
20:59 < wgrant> Because it was looking at the build status to generate the icon?
21:00 < jtv> wgrant: and because it was easier to have a link formatter for builds.
21:01 < wgrant> jtv: Argh, where's the template for that?
21:01 * wgrant cannot find it.
21:01 < jtv> builder-index.pt
21:03 < wgrant> jtv: Looks like buildfarmbuildjob-current.pt
21:03 < jtv> ah yes, that's where the link happens now.
21:03 < jtv> That was another thing: we can't show that icon for a buildless job.
21:04 < jtv> So that's another way in which it belongs to the build, not the builder.
21:04 < wgrant> So, I think we should do what is done for the rest of the states.
21:04 < wgrant> And just use the currentlybuilding icon if there's a job assigned.
21:04 < wgrant> Regardless of state.
21:04 < jtv> That's fine by me.
21:06 < wgrant> jtv: Ah, so you already hardcode that icon form non-build BFJs?
21:07 < jtv> wgrant: I _think_ I did that in an earlier iteration but it didn't seem appropriate.
21:07 < wgrant> It appears to be in devel's buildfarmjob-current.pt
21:08 < jtv> wgrant: there was a lot of exploring to get it all in a sort of minimal working shape... I don't mind moving things around a bit now that we have a working situation.
21:09 < wgrant> Yep.

20:55 < wgrant> jtv: The 'building' icon spacing on https://edge.launchpad.net/builders seems a bit broken. Is that related to your changes?
20:56 < jtv> wgrant: yes, it is.  How appropriate it is depends a bit on your perspective: it's the build status, so I'm displaying it as part of the link to the build.  It did simplify the code.
20:57 < jtv> wgrant: in fact this is the subject matter of a UI review that's currently somewhere in limbo.
20:57 < wgrant> jtv: The current spacing is definitely broken. I think the ordering is too, but that's up for debate.
20:58 < jtv> wgrant: I don't think I changed the ordering... did I?
20:58 < wgrant> It was '( ) Building _blah blah blah i386_'
20:58 < wgrant> It is now 'Building ( )_blah blah blah i386_'
20:59 < jtv> wgrant: oh, sorry, misunderstood you earlier.  That's the part I was talking about all along.
20:59 < wgrant> jtv: Why does it make the code simpler?
20:59 < wgrant> Because it was looking at the build status to generate the icon?
21:00 < jtv> wgrant: and because it was easier to have a link formatter for builds.
21:01 < wgrant> jtv: Argh, where's the template for that?
21:01  * wgrant cannot find it.
21:01 < jtv> builder-index.pt
21:03 < wgrant> jtv: Looks like buildfarmbuildjob-current.pt
21:03 < jtv> ah yes, that's where the link happens now.
21:03 < jtv> That was another thing: we can't show that icon for a buildless job.
21:04 < jtv> So that's another way in which it belongs to the build, not the builder.
21:04 < wgrant> So, I think we should do what is done for the rest of the states.
21:04 < wgrant> And just use the currentlybuilding icon if there's a job assigned.
21:04 < wgrant> Regardless of state.
21:04 < jtv> That's fine by me.
21:06 < wgrant> jtv: Ah, so you already hardcode that icon form non-build BFJs?
21:07 < jtv> wgrant: I _think_ I did that in an earlier iteration but it didn't seem appropriate.
21:07 < wgrant> It appears to be in devel's buildfarmjob-current.pt
21:08 < jtv> wgrant: there was a lot of exploring to get it all in a sort of minimal working shape... I don't mind moving things around a bit now that we have a working situation.
21:09 < wgrant> Yep.

« Back to merge proposal