launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

On Sat 04 Aug 2012 00:41:20 EST, Curtis Hovey wrote:
> Review: Needs Information code
>
> I think the failure handler on line 318 should use the red flash anim. Maybe this action never flashed red on failure. Oh I see the widget never flashed read on failure, but you can fix a stupid case where red was flashed on success.
> _unlink_bug_success on line 557 must flash green to state that the action was successful. I think this is another case where developer thought red me remove, when it actually means failure.
>
> I wonder if all the calls to
>     that.set('error', response.responseText)
> should instead call a method that does this an flash red. I will let you choose to decide if this should be done.
>
> The message on line 610 is ambiguous. Does it mean only some people will see the branch fixes the bug, or does it mean that the branch will be made private too?


The error handling uses the standard picker methodology. The 'error'
attribute is set and the red error text is displayed below the search
field. The picker still has focus so there's nothing to flash as such.
So I think we should leave it as is for now and at some other time look
at enhancing the standard picker error handing if necessary.

However, having said that, the error handling on line 464 is wrong. This
is when the unlink fails. There is no picker is use and the link that
was unsuccessfully removed should flash red. I'll fix this. And as you
say, if the unlink is successful, it should flash green before disappearing.

As for the privacy warning, how about:

'Linking this public branch to a private bug means that some
contributors may not see the bug fixed by this branch.'

If you are happy with these proposed fixes, and want to approve, I can
land when done.

-- 
https://code.launchpad.net/~wallyworld/launchpad/link-branch-to-bug-411409/+merge/118083
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.