1536030 - remove grid usage from comm/mail/components/preferences/fonts.xul, comm/mail/components/preferences/messengerLanguages.xul and comm/mail/components/preferences/receipts.xul

Status: RESOLVED FIXED

(Thunderbird :: Preferences, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: remove_grid_prefs_2.patch

To test messengerLanguages.xul: you need to remove hidden="true" from https://dxr.mozilla.org/comm-central/source/comm/mail/components/preferences/advanced.inc.xul#186.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9052301 [details] [diff] [review]
remove_grid_prefs_2.patch

Review of attachment 9052301 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/preferences/fonts.xul
@@ +95,5 @@
> +            <label accesskey="&monospace.accesskey;" control="monospace">&monospace.label;</label>
> +          </hbox>
> +        </vbox>
> +        <vbox>
> +          <menulist id="defaultFontType" style="width: 0px;">

I see this width 0 was already in the existing code. Looks strange though. Can it be removed?

@@ +276,5 @@
> +          </hbox>
> +        </vbox>
> +        <vbox>
> +          <menulist is="menulist-charsetpicker"
> +                    id="sendDefaultCharsetList"

especially for the is="" I think we want to keep at least that and id on one line for greppability

::: mail/components/preferences/messengerLanguages.xul
@@ +28,5 @@
>          class="prefpane largeDialogContainer"
>          flex="1">
>      <description data-l10n-id="messenger-languages-description"/>
> +    <hbox>
> +      <richlistbox id="selectedLocales" flex="1" height="250px"/>

instead of setting a specific height on the richlistbox, you should set flex="1"  on the <hbox> one line up

::: mail/components/preferences/receipts.xul
@@ +53,5 @@
> +          <hbox pack="end" flex="1">
> +            <label id="notInToCcLabel"
> +                   accesskey="&notInToCc.accesskey;"
> +                   control="notInToCcPref"
> +                   value="&notInToCc.label;"/>

these "If I'm not.... ", "If the sender... " and "In all other cases" are misaligned in the UI compared to earlier. Those label texts should all line up, not be right aligned towards the selection menu.

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: remove_grid_prefs_2.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9052543 [details] [diff] [review]
remove_grid_prefs_2.patch

Review of attachment 9052543 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with the below fixed. 
For the commit message, it's "Bug XXX - ...", not Bug - 
Also, please make it remove, not "removed"

::: mail/components/preferences/advanced.inc.xul
@@ +79,4 @@
>              </hbox>
>            </groupbox>
>  
> +          <groupbox id="messengerLanguagesBox">

this should stay hidden

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: remove_grid_prefs_2.patch

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9052719 [details] [diff] [review]
remove_grid_prefs_2.patch

Review of attachment 9052719 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/preferences/messengerLanguages.xul
@@ +56,5 @@
> +      <button id="add"
> +              class="add-messenger-language action-button"
> +              data-l10n-id="languages-customize-add"
> +              disabled="true"
> +              width="102px"/>

sorry I notice this now, but please don't hardcode widths. When you do, you're probably doing something wrong.

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: remove_grid_prefs_2.patch

Sorry, I forgot that to remove. This was the only patch where I had used the hard-coded width. Now, all of them are removed.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9052745 [details] [diff] [review]
remove_grid_prefs_2.patch

Review of attachment 9052745 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thx! r=mkmelin

Comment 9

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6d088f0d73dbaebf4a1497c459043a919b783c60

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fc8015e62158
remove grid usage from preferences' fonts.xul, messengerLanguages.xul and receipts.xul. r=mkmelin

Comment 11

[Jorg K (CEST = GMT+2)]

Can you please keep the commit messages reasonably short, no need to list the full path of all affected files ;-)