Code review comment for lp:~wallyworld/launchpad/recipe-build-now

Hi Henning

Thanks for the detailed feedback. It's appreciated.

On 17/02/11 17:56, Henning Eggers wrote:
> Review: Needs Fixing ui
> Hi Ian,
> thank you for this simple fix and taking the time to include some ajax and graceful degradation. It seems to work nicely but I do have a couple of issues I'd like to see addressed:
>
> - I am not sure why the non-ajax case needs to include a button and does not get the nice new icon. Do we do that in other places already? The original idea was that ajax-enabled links are green, others blue and I think that still holds. This may be different here, though, because the link does not lead to a new page but simply starts an action on the same page. So the button may be OK if it is for that reason.

A form submission (post) is required for this operation in order to
start a write transaction. The issue is that without javascript a form
submission can be done with either a button or an image, but not a link.
In other similar places to this, we use a button (eg the vote or claim
review buttons on the mp page). Since buttons are used elsewhere for
this type of operation, I stuck with that convention. The alternative
was to go with an image that is done to look like a button or similar
but that's not done anywhere else. Because a post is required, a (blue)
link is not sufficient since that only does a get and starts a readonly
transaction.

> - The non-ajax case definitely needs some feed back. This is usually achieved by using the message boxes at the top of the page. "The build has been requested, see the Latest Builds table below." or something like that.

Good idea. Thanks. I'll do that.

> - The ajax case has a nice "requesting build" spinner when you click it but then just simply disappears. This leaves the user at a loss if the operation succeeded or not. (Can it fail btw?) A possible feedback might be to flash the Latest Builds table green, or rather the lines of the builds being started.

Any new builds rows are indeed flashed green using the standard yui
anim() call that we use in lp. Note that if any daily builds which
result from the Build Now click are already pending, they are not
reflashed. However this should be very rare,since the Build Now only
appears if the recipe is stale. Perhaps you missed seeing the row animation?

> - The feed-back problem also made me think if the button is placed correctly in the first place. I actually think it should be placed by the table (which represents operational data about the builds) and not in the recipe information (which represents static data about the recipe). While writing this, I became very sure that it has to be moved. It should go right under the "Latest builds" heading in a line expressing the current state ("it is stale" but worded better ;).
>

Ok. I'll mock something up and see how that looks.

> On the whole this fix is a really nice start but I have to ask you to please fix the feed-back deficiencies before I can sign this off.
>

No problem. The feedback was very good. Thanks.