Closed
Bug 1536030
Opened 6 years ago
Closed 6 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)
Thunderbird
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 68.0
People
(Reporter: khushil324, Assigned: khushil324)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
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 2•6 years ago
|
||
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="¬InToCc.accesskey;" > + control="notInToCcPref" > + value="¬InToCc.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-
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9052301 -
Attachment is obsolete: true
Attachment #9052543 -
Flags: review?(mkmelin+mozilla)
Comment 4•6 years ago
|
||
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+
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9052543 -
Attachment is obsolete: true
Attachment #9052719 -
Flags: review?(mkmelin+mozilla)
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
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 8•6 years ago
|
||
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+
Assignee | ||
Comment 9•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 10•6 years ago
|
||
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
Comment 11•6 years ago
|
||
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
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•