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

Revision history for this message
Henning Eggers (henninge) wrote :

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.
- 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.
- 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.
- 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 ;).

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.


review: Needs Fixing (ui)

« Back to merge proposal