Closed Bug 1536028 Opened 6 years ago Closed 6 years ago

remove grid usage from comm/mail/components/preferences/cookies.xul and comm/mail/components/preferences/display.inc.xul

Categories

(Thunderbird :: Preferences, task)

Product:
Thunderbird
Component:
Preferences
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 4 obsolete files)

remove_grid_prefs_1.patch
(deleted), patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → khushil324
Status: NEW → ASSIGNED
Depends on: tb-burn-xul-grids
Blocks: tb-burn-xul-grids
No longer depends on: tb-burn-xul-grids
Attachment #9051643 - Attachment description: Bug 1536028 - Vendor pytest-mock via |mach vendor python|. r=ahal → Bug 1536029 - Vendor pytest-mock via |mach vendor python|. r=ahal
Attached patch remove_grid_prefs_1.patch (obsolete) (deleted) — — Splinter Review
Attachment #9052070 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9052070 [details] [diff] [review] remove_grid_prefs_1.patch Review of attachment 9052070 [details] [diff] [review]: ----------------------------------------------------------------- I don't know what the `width="*"` do, but you might want to check if that is compatible with all locales. ::: mail/components/preferences/display.inc.xul @@ +22,5 @@ > <label><html:h2>&fontsAndColors1.label;</html:h2></label> > > + <vbox id="fontsGrid"> > + <hbox id="fontRow"> > + <hbox flex="1" align="center"> Not sure if `<hbox flex="1" align="center">` makes a difference here, you might be able to remove that, and make everything a direct child of `#fontRow` (and add align="center" there if needed). @@ +63,5 @@ > + <button id="advancedFonts" label="&fontOptions.label;" icon="select-font" accesskey="&fontOptions.accesskey;" oncommand="gDisplayPane.configureFonts();"/> > + </hbox> > + <hbox id="colorsRow"> > + <hbox flex="1"/> > + <button style="width: 99px" id="colors" icon="select-color" label="&colorButton.label;" accesskey="&colorButton.accesskey;" oncommand="gDisplayPane.configureColors();"/> You can drop `<hbox flex="1"/>` and use pack="end" on #colorsRow
Comment on attachment 9052070 [details] [diff] [review] remove_grid_prefs_1.patch Review of attachment 9052070 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/cookies.xul @@ +54,4 @@ > <treechildren id="cookiesChildren"/> > </tree> > <hbox id="cookieInfoBox"> > + <vbox flex="1" id="cookieInfoGrid"> I wonder if just using <html:table> would work for this It's basically table information @@ +58,5 @@ > + <hbox align="center"> > + <hbox pack="end" width="65"> > + <label id="nameLabel" control="name" value="&props.name.label;"/> > + </hbox> > + <textbox flex="1" id="name" readonly="true" class="plain"/> please keep ids first @@ +61,5 @@ > + </hbox> > + <textbox flex="1" id="name" readonly="true" class="plain"/> > + </hbox> > + <hbox align="center"> > + <hbox pack="end" width="65"> hardcoding a 65 looks wrong @@ +94,5 @@ > + <hbox align="center" id="userContextRow"> > + <hbox pack="end" width="65"> > + <label id="userContextLabel" control="userContext" value="&props.container.label;"/> > + </hbox> > + <textbox flex="1" id="userContext" readonly="true" class="plain"/> pleased keep the ids first (except for is=) ::: mail/components/preferences/display.inc.xul @@ +21,4 @@ > <groupbox id="fontsGroup"> > <label><html:h2>&fontsAndColors1.label;</html:h2></label> > > + <vbox id="fontsGrid"> rename the id to fontSettings @@ +63,5 @@ > + <button id="advancedFonts" label="&fontOptions.label;" icon="select-font" accesskey="&fontOptions.accesskey;" oncommand="gDisplayPane.configureFonts();"/> > + </hbox> > + <hbox id="colorsRow"> > + <hbox flex="1"/> > + <button style="width: 99px" id="colors" icon="select-color" label="&colorButton.label;" accesskey="&colorButton.accesskey;" oncommand="gDisplayPane.configureColors();"/> I don't think the button should have a style width like this. I doubt that would be good on localizations. In general, could have a look at what Firefox has: https://searchfox.org/comm-central/source/mozilla/browser/components/preferences/in-content/main.xul#136
Attachment #9052070 - Flags: review?(mkmelin+mozilla)
Attachment #9052070 - Flags: review-
Attachment #9052070 - Flags: feedback+
Attached patch remove_grid_prefs_1.patch (obsolete) (deleted) — — Splinter Review
Attachment #9052070 - Attachment is obsolete: true
Attachment #9052611 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9052611 [details] [diff] [review] remove_grid_prefs_1.patch Review of attachment 9052611 [details] [diff] [review]: ----------------------------------------------------------------- While it looks nice, the hard coded widths are scary and likely the wrong thing to do. For connection.xul, the de-grid of browser/components/preferences/connection.xul hasn't happened yet. We're basically just keeping our copy up do date with that, so it could actually be worth just not including that part in this patch. Also, probably preferable to just do one file per patch/bug (for future reference). Please also fix the commit message ::: mail/components/preferences/connection.xul @@ +38,5 @@ > id="systemPref" hidden="true"/> > <radio value="1" label="&manualTypeRadio.label;" accesskey="&manualTypeRadio.accesskey;"/> > + <vbox class="indent" flex="1"> > + <hbox align="center"> > + <hbox pack="end" width="100"> no width please @@ +51,5 @@ > + onsyncfrompreference="return gConnectionsDialog.readHTTPProxyPort();"/> > + </hbox> > + </hbox> > + <hbox> > + <hbox width="104"/> and here + a few more @@ +82,5 @@ > + preference="network.proxy.socks" > + onsyncfrompreference="return gConnectionsDialog.readProxyProtocolPref('socks', false);"/> > + <label value="&SOCKSport.label;" accesskey="&SOCKSport.accesskey;" control="networkProxySOCKS_Port"/> > + <textbox id="networkProxySOCKS_Port" type="number" max="65535" size="5" > + preference="network.proxy.socks_port" misaligned with one char it seems
Attachment #9052611 - Flags: review?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #7)

Comment on attachment 9052611 [details] [diff] [review]

For connection.xul, the de-grid of
browser/components/preferences/connection.xul hasn't happened yet. We're
basically just keeping our copy up do date with that, so it could actually
be worth just not including that part in this patch. Also, probably
preferable to just do one file per patch/bug (for future reference).

So should I just remove connection.xul changes from this file right now? Should I open the new bug for that now?

I think that's easiest. We can file a new bug later.

Cool, will update the patch in a while.

Summary: remove grid usage from comm/mail/components/preferences/connection.xul, comm/mail/components/preferences/cookies.xul and comm/mail/components/preferences/display.inc.xul → remove grid usage from comm/mail/components/preferences/cookies.xul and comm/mail/components/preferences/display.inc.xul
Attached patch remove_grid_prefs_1.patch (obsolete) (deleted) — — Splinter Review
Attachment #9052611 - Attachment is obsolete: true
Attachment #9052863 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9052863 [details] [diff] [review] remove_grid_prefs_1.patch Review of attachment 9052863 [details] [diff] [review]: ----------------------------------------------------------------- Please fix the commit message now that connection.xul is not included ::: mail/components/preferences/display.inc.xul @@ +28,5 @@ > + <menulist id="defaultFont" flex="1" sizetopopup="pref" crop="center" onsyncfrompreference="return gDisplayPane.readFontSelection()"> > + <menupopup crop="center"/> > + </menulist> > + <label accesskey="&defaultSize.accesskey;" control="defaultFontSize">&defaultSize.label;</label> > + <menulist flex="1" id="defaultFontSize"> id first please
Attachment #9052863 - Flags: review?(mkmelin+mozilla) → review+
Attached patch remove_grid_prefs_1.patch (deleted) — — Splinter Review
Attachment #9052863 - Attachment is obsolete: true
Attachment #9052972 - Flags: review?(mkmelin+mozilla)
Keywords: checkin-needed

Magnus, can you please check the line length. There are lines over 180 characters(!!!) long, that doesn't comply with any coding standard :-( - I will shorten them now.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f9d9f70bc490
remove grid usage from preferences' cookies.xul and display.inc.xul. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
Attachment #9052972 - Flags: review?(mkmelin+mozilla)
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

Creator:
Created:
Updated:
Size: