Closed Bug 643262 Opened 10 years ago Closed 10 years ago

Update the appearance of the Contacts sidebar

Categories

(Thunderbird :: Theme, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 6.0

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(2 files, 3 obsolete files)

The Contacts sidebar does not fit well with the new appearance in Aero theme. I think this needs an overhaul.
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.
Attachment #520517 - Flags: feedback?(nisses.mail)
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.
ie. the global toolkit close in mozilla/toolkit/themes/winstripe/global/icons rather than the one in mail/themes/qute/mail/icons
I'm waiting to make a patch until Aero Glass lands. I'll then using the global close button.
(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 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.
Attachment #520517 - Flags: feedback?(nisses.mail) → feedback+
Attached patch The patch V1 (obsolete) — Splinter Review
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
Attachment #529673 - Flags: ui-review?(nisses.mail)
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
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+
Attached patch The patch V2 (obsolete) — Splinter Review
Patch updated after landing of Bug 654916
Attachment #529673 - Attachment is obsolete: true
Attachment #529673 - Flags: ui-review?(nisses.mail)
Attachment #531148 - Flags: ui-review?(nisses.mail)
Comment on attachment 531148 [details] [diff] [review]
The patch V2

Looks good!
Attachment #531148 - Flags: ui-review?(nisses.mail) → ui-review+
Attachment #531148 - Flags: review?(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.
Attachment #531148 - Flags: review?(bwinton) → review+
(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.
Attachment #531148 - Attachment is obsolete: true
Keywords: checkin-needed
(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.
I added the boilerplate before the include. Then both boilerplates should be together, if both are used.
Attachment #534204 - Attachment is obsolete: true
Checked in: http://hg.mozilla.org/comm-central/rev/02c7badcf034
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.4
You need to log in before you can comment on or make changes to this bug.