Closed Bug 1536030 Opened 5 years ago Closed 5 years ago

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

Categories

(Thunderbird :: Preferences, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → khushil324
Status: NEW → ASSIGNED
Attached patch remove_grid_prefs_2.patch (obsolete) — Splinter Review

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.

Attachment #9052301 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9052301 - Flags: review?(mkmelin+mozilla) → review-
Attached patch remove_grid_prefs_2.patch (obsolete) — Splinter Review
Attachment #9052301 - Attachment is obsolete: true
Attachment #9052543 - Flags: review?(mkmelin+mozilla)
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
Attachment #9052543 - Flags: review?(mkmelin+mozilla) → review+
Attached patch remove_grid_prefs_2.patch (obsolete) — Splinter Review
Attachment #9052543 - Attachment is obsolete: true
Attachment #9052719 - Flags: review?(mkmelin+mozilla)
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.

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.

Attachment #9052719 - Attachment is obsolete: true
Attachment #9052719 - Flags: review?(mkmelin+mozilla)
Attachment #9052745 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9052745 [details] [diff] [review]
remove_grid_prefs_2.patch

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

LGTM, thx! r=mkmelin
Attachment #9052745 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

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

Target Milestone: --- → Thunderbird 68.0
Depends on: 1540423
No longer depends on: 1540423
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: