1568021 - remove grid usage from comm/mailnews/base/content/msgAccountCentral.xul

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568021_remove-grid-msgAccountCentral.patch

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9079838 [details] [diff] [review]
Bug-1568021_remove-grid-msgAccountCentral.patch

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

This is mostly good, just some nits to change before granting the review.

Please, ask Magnus to review it after the requested changes as I won't be back online until Tuesday next week.

::: mailnews/base/content/msgAccountCentral.xul
@@ +31,4 @@
>    <script src="chrome://messenger/content/msgAccountCentral.js"/>
>    <script src="chrome://messenger/content/mailCore.js"/>
>  
> +  <hbox id="acctCentralLayout" flex="1" style="overflow: auto;">

Move the `overflow: auto;` attribute into the #acctCentralLayout selector

@@ +46,3 @@
>  
>        <spacer id="ReadMessages.spacer" flex="1" collapsed="true"/>
> +      <hbox id="ReadMessages" class="acctCentralRow" collapsed="true">

I know you didn't do this but let's use camel case for the IDs, not pascal case.
So, lowercase for the first word, eg. readMessages.
Update all the IDs and CSS selectors accordingly.

Comment 3

[Magnus Melin [:mkmelin]]

I think for the classes, use lower case names with dashes.

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568021_remove-grid-msgAccountCentral.patch

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9080955 [details] [diff] [review]
Bug-1568021_remove-grid-msgAccountCentral.patch

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

LGTM, r=mkmelin

Comment 6

[Jorg K (CEST = GMT+2)]

What's the point of all this code churn? You wanted to replace grids and started renaming an awful lot of identifiers. I can't see the forest for the trees. If you really must do that, split it into two patches.

Magnus, is this really necessary?

Comment 7

[Magnus Melin [:mkmelin]]

I blame aleca ;)
Anyway, the capitalized ids were very ugly so let's just go with what we got.
I notice now, there were some ids with dots in them (like offlineSettings.spacer), please fix that (->offlineSettingsSpacer)

Comment 8

[Jorg K (CEST = GMT+2)]

As per the discussion on "atomic" changes: Let's not throw the essential part of grid removal into the same patch as the wholesale renaming. I hope you didn't miss anything.

Comment 9

[Magnus Melin [:mkmelin]]

Yes but it's already done now and splitting it up would be pretty pointless.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568021_remove-grid-msgAccountCentral.patch

Comment 11

[Jorg K (CEST = GMT+2)]

I'm splitting this now as I had requested. Took less than 30 minutes.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Bug-1568021_remove-grid-msgAccountCentral-in-xul.patch

These are the net XUL changes. Wholesale rename will be in a second patch.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Bug-1568021_wholesale-rename.patch

Here's the rename part.

Why are we using different styles for IDs and classed, looks terrible :-( - like:

-      <spacer id="SubscribeImapFolders.spacer" flex="1" collapsed="true"/>
-      <hbox id="SubscribeImapFolders" class="acctCentralRow" collapsed="true">
+      <spacer id="subscribeImapFoldersSpacer" flex="1" collapsed="true"/>
+      <hbox id="subscribeImapFolders" class="acct-central-row" collapsed="true">

Comment 14

[Khushil Mistry [:khushil324]]

Comment on attachment 9081871 [details] [diff] [review]
Bug-1568021_wholesale-rename.patch

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

Let me add the two seprate patches.

::: mail/themes/shared/mail/accountCentral.css
@@ +33,3 @@
>    background-color: var(--body-background-color);
>    color: var(--body-text-color);
>  }

We want this CSS change in the patch above, not in this.

@@ +52,5 @@
>  }
>  
> +.acct-central-title-row {
> +  padding-left: 10px;
> +}

This also.

@@ +78,3 @@
>    margin-inline-start: 6px;
>  }
>  

This also.

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568021_remove-grid-msgAccountCentral-xul.patch

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Khushil Mistry [:khushil324] from comment #14)

Let me add the two seprate patches.

That's what I asked for in comment #6 :-) - Sorry I got the CSS distribution wrong.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9f87d233392f
remove grid usage from msgAccountCentral.xul. r=mkmelin