Code review comment for lp:~henninge/launchpad/bug-422466-status-picker

> Hi Henning.

Great work Henning! This will be an excellent precedent for other call-sites that need dynamically disabled options like this.

Sorry if it's confusing below, I started writing before you pushed your changes. I'm setting this to approve, but only with Curtis' feedback/approval also - there are two points below that I'm keen to hear his thoughts on.

>
> I really the addition of the picker, and your considerations of how to deal
> with unavailable status. I have a few concerns that this addition has
> exacerbated. The edit/info Translation Queue Entry links looks like they is
> separated from the content. With the addition of inline editing, the layout
> looks like a mistake.

I agree with Curtis here (and you - as you mentioned you foresaw this too).

>
> I think the icon to edit the translation queue entry must be closer to the
> parts it modifies. Consider creating the link twice and placing it after the
> "pot" and "template" parts because that tells me I can change those aspects of
> the entry.

That's a good solution that will mean the UI doesn't need to change at all when we eventually in-line those items too. I do think it's worth having the link in both places as suggested, rather than just after the template (as in your recent revs to fix this), although, you then mentioned that it is really only the translation domain that is edited. I'll leave that up to you - you know the usage of the page and can evaluate which option is better in the mean-time.

If you do decide to only have it in the one place, I wonder whether it would be best just to the right of "po/evolution-2.2-test.pot in Evolution trunk series" so it implies that it edits the whole item rather than just the template? (on the other hand, this could imply that it just edits the series). What do you think Curtis?

As an aside, is there a reason why we're not using columns with headers for the main info in this table? Like:

| Uploaded file | Package/Branch (?) | Uploaded by | Status |

or similar? (I'd still leave the template section where it is, especially if it's optional, as the form implies, otherwise the columns would be too wide and too much data).

>
> While pondering a suggestion to fix this, I discovered that the info button is
> attached the expander, and it expands content that it is not pointing to. This
> is wrong. Every OS I have seen that uses an expander places it to the left of
> the content so that the arrows point to where your eyes look (like a
> checkbox). I toggled several times to learn what was being expanded. The
> expander should be to the left of the entry. I believe this is a separate bug
> that should not prevent you from landing this. If a bug is not reported,
> please do.

+ 1. I actually had to check on edge to see what the info button did. When I clicked locally I saw:

{{{
new_button.attach is not a function
[Break on this error] new_button.attach('click', (shown ? hide_output : show_output));\n
}}}

(OK, you mentioned on IRC that you get this too).

If you do create a bug for the expander, I think it would be much more obvious if you had the following under the "Will be imported into..." (currently when expanded it is above the Uploaded by...).

Expander> Import log details (or something appropriate to indicate what it is that will be shown)

Regarding the colour selection - as mentioned, I initially had trouble distinguishing between some options and the disabled option. You since fixed that by using a different colour scheme. Curtis can perhaps check the new colours but to my eyes, it's certainly easier to see that they're *not* disabled :-)