643262 - Update the appearance of the Contacts sidebar

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

The Contacts sidebar does not fit well with the new appearance in Aero theme. I think this needs an overhaul.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: Comparsion of old and proposed Contacts sidebar

On the left the actual style and on the right the proposed styling (already with Aero Glass patch).

What I changed: 
- sidebar header equal to the FolderPane header.
- background matching the MessageHeaders toolbar.
- added a bottom border to the address tree to separate it from the button on bottom.
- sidebar splitter equal to the FolderPane splitter.

Comment 2

[Andreas Nilsson (:andreasn)]

Looks great!
ui-r+ if you fix the close button in the process as well to make it the same as we use in the tabs and that Firefox uses in the Bookmarks sidebar.

Comment 3

[Andreas Nilsson (:andreasn)]

ie. the global toolkit close in mozilla/toolkit/themes/winstripe/global/icons rather than the one in mail/themes/qute/mail/icons

Comment 4

[Richard Marti (:Paenglab)]

I'm waiting to make a patch until Aero Glass lands. I'll then using the global close button.

Comment 5

[Andreas Nilsson (:andreasn)]

(In reply to comment #4)
> I'm waiting to make a patch until Aero Glass lands. I'll then using the global
> close button.

Sounds like a good idea. Lets land bug 569400 before we push this.

Comment 6

[Andreas Nilsson (:andreasn)]

Comment on attachment 520517 [details]
Comparsion of old and proposed Contacts sidebar

Hadn't put feedback+ here.
This is good, provided the close button gets fixed.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: The patch V1

In this patch is all I wrote in Comment 1. 
Additionally:
- using the close button from global/icons
- gave Add to To:, Add to CC: and Add to BCC: buttons the toolbar-button look
- compose-toolbar-sizer is invisible but usable like in mockup (attachment 519886 [details])
- attachmentbucket-sizer has the same look as the other splitter, but is 2px wide
- gave content under Aero a border like in Bug 638407

Comment 8

[Andreas Nilsson (:andreasn)]

I like all the changes!
One odd thing I stumbled upon was that resizing behaves a bit odd now (but perhaps it has for some time), will double-check to make sure this is not caused by your patch before I give a ui-r+

Comment 9

[Richard Marti (:Paenglab)]

Attachment: The patch V2

Patch updated after landing of Bug 654916

Comment 10

[Andreas Nilsson (:andreasn)]

Comment on attachment 531148 [details] [diff] [review]
The patch V2

Looks good!

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 531148 [details] [diff] [review]
The patch V2

>+++ b/mail/themes/qute/mail/compose/messengercompose-aero.css
>@@ -278,6 +282,16 @@
>+  #sidebar-splitter:not([hidden="true"]) + vbox >
>+                                     #headers-box:-moz-locale-dir(ltr) {
>+    border-top-left-radius: 0;
>+  }
>+
>+  #sidebar-splitter:not([hidden="true"]) + vbox >
>+                                     #headers-box:-moz-locale-dir(rtl) {

That's some odd spacing going on there.  How would you like to just use four spaces instead?

>+++ b/mail/themes/qute/mail/addrbook/abContactsPanel-aero.css
>@@ -0,0 +1,68 @@

I think we need to include the standard license boilerplate here.

>+%include abContactsPanel.css
>+button,
>+#addressbookList {
>+  -moz-appearance: none;
>+  padding: 1px 5px !important;
>+  background: rgba(151, 152, 153, .05)

Are you missing a comma here?

>+              -moz-linear-gradient(rgba(251, 252, 253, .95),
>+              rgba(246, 247, 248, .47) 49%, rgba(231, 232, 233, .45) 51%,
>+              rgba(225,226,229,.3));

I think these two lines should be indented a little more, cause they're part of the linear gradient.

But that's all pretty minor, so r=me with the nits fixed.

Thanks,
Blake.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: Patch for check-in addressing comments

(In reply to comment #11)
> Comment on attachment 531148 [details] [diff] [review] [review]
> The patch V2
> 
> >+++ b/mail/themes/qute/mail/compose/messengercompose-aero.css
> >@@ -278,6 +282,16 @@
> >+  #sidebar-splitter:not([hidden="true"]) + vbox >
> >+                                     #headers-box:-moz-locale-dir(ltr) {
> >+    border-top-left-radius: 0;
> >+  }
> >+
> >+  #sidebar-splitter:not([hidden="true"]) + vbox >
> >+                                     #headers-box:-moz-locale-dir(rtl) {
> 
> That's some odd spacing going on there.  How would you like to just use four
> spaces instead?

Made only four spaces.

> >+++ b/mail/themes/qute/mail/addrbook/abContactsPanel-aero.css
> >@@ -0,0 +1,68 @@
> 
> I think we need to include the standard license boilerplate here.

abContactsPanel.css has already a boilerplate and abContactsPanel-aero.css will be appended to it. This would result in a second boilerplate in the midde of the file. Or am I wrong and the processing is smart enough to remove it?

> >+%include abContactsPanel.css
> >+button,
> >+#addressbookList {
> >+  -moz-appearance: none;
> >+  padding: 1px 5px !important;
> >+  background: rgba(151, 152, 153, .05)
> 
> Are you missing a comma here?

No, background: bg-color image ... has also no comma. Here it's the same: bg-color=rgba(151, 152, 153, .05) and image=-moz-linear-gradient(...
> >+              -moz-linear-gradient(rgba(251, 252, 253, .95),
> >+              rgba(246, 247, 248, .47) 49%, rgba(231, 232, 233, .45) 51%,
> >+              rgba(225,226,229,.3));
> 
> I think these two lines should be indented a little more, cause they're part
> of the linear gradient.

I gave two spaces indentation.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #12)
> > >+++ b/mail/themes/qute/mail/addrbook/abContactsPanel-aero.css
> > I think we need to include the standard license boilerplate here.
> abContactsPanel.css has already a boilerplate and abContactsPanel-aero.css
> will be appended to it. This would result in a second boilerplate in the
> midde of the file. Or am I wrong and the processing is smart enough to
> remove it?

I doubt it is smart enough, but it'll be a CSS comment, so it shouldn't harm the combined file.  And legally, I believe it needs to be at the top of every source file, irrespective of whether or not that file gets included in other files.

For instance, there's boilerplate at the top of http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/public/nsNetUtil.h even though it's included in http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp which also has boilerplate of its own.

So, please add that, and then we can get this patch checked in.

Thanks,
Blake.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Patch for check-in

I added the boilerplate before the include. Then both boilerplates should be together, if both are used.

Comment 15

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/02c7badcf034