Closed Bug 1568021 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
Assignee: nobody → khushil324
Attachment #9079838 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attachment #9079838 - Flags: review?(mkmelin+mozilla) → review?(alessandro)
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.
Attachment #9079838 - Flags: review?(alessandro) → review-

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

Attachment #9079838 - Attachment is obsolete: true
Attachment #9080955 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9080955 [details] [diff] [review]
Bug-1568021_remove-grid-msgAccountCentral.patch

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

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

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?

Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed

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)

Flags: needinfo?(mkmelin+mozilla)

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.

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

Attachment #9080955 - Attachment is obsolete: true
Attachment #9081864 - Flags: review+
Keywords: checkin-needed

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

Keywords: checkin-needed

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

Attachment #9081864 - Attachment is obsolete: true
Attachment #9081869 - Flags: review?(khushil324)

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">
Attachment #9081871 - Flags: review?(mkmelin+mozilla)
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.
Blocks: 1570233
Attachment #9081869 - Attachment is obsolete: true
Attachment #9081871 - Attachment is obsolete: true
Attachment #9081869 - Flags: review?(khushil324)
Attachment #9081871 - Flags: review?(mkmelin+mozilla)
Attachment #9081881 - Flags: review+
Keywords: checkin-needed

(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.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9f87d233392f
remove grid usage from msgAccountCentral.xul. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.