Code review comment for lp:~frankban/juju-quickstart/improve-field-choices

Revision history for this message
Gary Poster (gary) wrote :

QA first:

- Very cool. Looks good generally.

- It looks like Urwid can support an "underline" setting. I think that
would make it easier to see the clickable help text (for both
autogeneration and these choices). WDYT?

- In my default terminal width (80x24) I see "possible values are:
precise, quantal, raring, saucy". If I use the arrow keys then I can
see there is a final option (", but this field can also be _left
empty_"). With the mouse I am out of luck. I'd like to improve that if
we could. Options include letting the line wrap, if Urwid supports
that; showing a clickable "MORE->" or "..." or something; or simply
trying to reduce the length of the clickable text.

In line with the last, simplest idea, what about replacing the four
lines with something like the following? I'm assuming we can make the
first text wrappable easily, but if not maybe this idea needs tweaking.

To select a value, type above, _clear the field_ to get the default
value (precise), or click one of the following choices:
_precise_, _quantal_, _raring_, _saucy_, _trusty_

Mm. Darn. I see in the region field that trying to reduce the length
of the clickable text is insufficient. Those options go on for days.
That will need line wrap or something else similar

- Relatedly, and probably for the future (and maybe never), it would be
nice if the fields that can be automatically generated are allowed to be
empty, which defaults to using the autogeneration.

- I see that we actually explicitly set precise in the file if it is
empty (as you describe in the cover letter). However, IIUC, Juju will
work fine if you do not specify a series. The current *Juju* default
behavior is to default to precise, but probably will change to
defaulting to trusty eventually. That makes me think for our
opinionated usage that we should begin rendering the field with
"precise" but if someone explicitly deletes the field then it is simply
empty (and the "empty" semantics are described in the fields help text).
  WDYT?

Thank you!

https://codereview.appspot.com/34630044/

« Back to merge proposal