Comment 94 for bug 115484

(In reply to comment #85)
> (From update of attachment 346231 [details])
> git-apply complained of 32 whitespace errors, of which at least 25 are new.
> If you can't find them try /msg firebot review 346231 (note: it's designed to
> review C++ so some of its other quibbles aren't always appropriate.)
Thanks for the hint. I removed the whitespace and longline errors but there are 4 left in the current patch. I have changed it locally here for the moment.
>
> I thought the display looked a little crowded; the current version at least has
> those groupboxes that break up the dialog into manageable pieces.
I removed the groupboxes on purpose because they consume quite some space. Because I address is now split in home and business, that one is obsolete. For the phone numbers and Emails on the first tab, I compared it to Outlook where they also have it like this. So in the end I removed them only because of the space.
> >+<!ENTITY allowRemoteContent.tooltip "In HTML messages it is possible to embed images from remote sources. Opening
> >+such a message will open a connection to this external source. This may allow
> >+tracking of the message being read. Checking this box will allow such external
> >+embedded images in HTML messages from this contact." >
> I don't know how common wrapping long entities is, but you've wrapped the text
> in the entity rather than the text in the file, which looks odd. Also speaking
> of whitespace errors you can lose the space before the >
I split it a little differently to have no line longer than 80 but I left it essentially.
>
> > .CardEditWidth {
> >- width: 25em;
> >+ width: 21em;
> Is it possible to change these widths to the equivalent in ch?
> (This affects themes/platforms that use a bold font)
Done.
>
> > .alignBoxWithFieldset {
> > margin-left: 6px;
> > margin-right: 5px;
> > }
> You don't use this any more. (Both comments apply for all themes of course.)
Removed it.
>
> >+ <hbox id="nickNameContainer">
> >+ <spacer flex="1"/>
> >+ <label control="NickName" value="&NickName.label;"
> >+ accesskey="&NickName.accesskey;" class="CardEditLabel"/>
> >+ <hbox class="CardEditWidth">
> >+ <textbox id="NickName" flex="1"/>
> > </hbox>
> > </hbox>
> Just picking on this one because it was easy to extract on the diff, but this
> applies to most of the fields in this window.
> Rather than setting the top margin in the CardEditLabel class, I think the
> outer hbox needs an align="center" so the labels line up properly with the
> textboxes.
> If you wanted to, you could get rid of the <spacer> by using <label flex="1">
> and adding text-align: right; to the CardEditLabel class.
I removed the CardEditLabel class and did the alignment with the attribute.
>
> >- <caption>
> >- <label control="Notes" value="&Notes.box;" accesskey="&Notes.accesskey;"/>
> >- </caption>
> This caption was important as it labelled the Notes control...
I added the label again.

Thanks for the review!