The default bug view has changed. See this FAQ.

make the "Add new calendar" icon the same height of 24px as the other icons in the Account central page.

RESOLVED FIXED in Thunderbird 17.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({polish})

Trunk
Thunderbird 17.0
polish
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #681735 comment 22 +++

The icon for the "add a new calendar" action in the Account central page uses a 32px high icon so it is higher than other icons on the page. The action has bigger padding than the other actions.

There was a patch proposal in bug 681735 but it is moved here so that landing rest of bug 681735 is not blocked by arguing about the exact icon used.

Here is a screenshot of how the action with a 24px icon would look like if the patch here is applied: https://bugzilla.mozilla.org/attachment.cgi?id=625482 (the icon is not necessarily the one in the patch, it is just an illustration).

The current patch didn't pass review in bug 681735 comment 32.
So I need further proposals.
(Assignee)

Comment 1

5 years ago
Created attachment 626901 [details] [diff] [review]
patch

This is the patch. Review declined by Fallen:

Philipp Kewisch [:Fallen] (away until May 28th) 2012-05-24 19:17:48 CEST

Comment on attachment 626596 [details] [diff] [review] [diff] [review]
patch v3

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

f- since I don't think we should use the toolbar icons.

::: calendar/lightning/themes/common/suite-accountCentral.css
@@ +37,5 @@
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
>  #lightning-newCalendar-row {
> +  list-style-image: url("chrome://calendar/skin/toolbar-large.png");
> +  -moz-image-region: rect(0px 168px 24px 144px);

IIRC then mac uses 24px icons in the toolbar, so setting this for suite might fail. I think its best to use a 24x24px version of the cal-icon32.png.

::: calendar/lightning/themes/pinstripe/accountCentral.css
@@ +34,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
>  #lightning-newCalendar-row > hbox > label {
> +  background: -moz-image-rect(url(chrome://calendar/skin/toolbar-small.png), 0, 168, 24, 144) no-repeat !important;

As mentioned, mac positions might be wrong
Attachment #626901 - Flags: ui-review-
(Assignee)

Comment 2

5 years ago
If really a new icon is wanted (I always wanted just to reuse some existing ones), then somebody needs to create them. I can then wire them up.

Comment 3

5 years ago
> Yeah, I wondered why there is only one definition for suite for all platforms, when
> TB had 3.
Like you I can only test on one platform and I needed to get something working for SeaMonkey.
As far as I can see, this bug only affects Windows and Linux, as calendar/base/themes/pinstripe/images/cal-icon32.png is the same size as the icons in mail/themes/pinstripe/mail/accountcentral (although the icons on pinstripe gets cut at the bottom, but that's yet another bug).
Created attachment 627689 [details] [diff] [review]
24x24 icons for gnomestripe and winstripe

Attached are new icons in calendar/base/themes/*/images for gnomestripe and winstripe (and in pinstripe too, even though not needed, since that's how jar.nm seems to be set up).
(Assignee)

Comment 6

5 years ago
Thanks. Maybe now when the icon filename is the same, the one definition for Seamonkey will work for all the themes.

Should I merge this into my patch?
(Assignee)

Comment 7

5 years ago
Andreas, while you are in icon creation mode, I think you have not yet responded to bug 727598 :)
(In reply to :aceman from comment #6)
> Thanks. Maybe now when the icon filename is the same, the one definition for
> Seamonkey will work for all the themes.
> 
> Should I merge this into my patch?

Yes!

Comment 9

5 years ago
Have these icons been run through the likes of OptiPNG?
(Assignee)

Comment 10

5 years ago
(In reply to Andreas Nilsson (:andreasn) from comment #4)
> (although the icons on
> pinstripe gets cut at the bottom, but that's yet another bug).
Can you attach a screenshot of the icon cut-off?
Also, does it get fixed if you add 
"min-height: 32px" to #acctCentralLinkText in pinstripe?
(Assignee)

Comment 11

5 years ago
Created attachment 627766 [details] [diff] [review]
patch with new icons

(In reply to Andreas Nilsson (:andreasn) from comment #5)
> Attached are new icons in calendar/base/themes/*/images for gnomestripe and
> winstripe (and in pinstripe too, even though not needed, since that's how
> jar.nm seems to be set up).

So I just copied cal-icon32.png to cal-icon.png in pinstripe so that the one definition for suite (url(cal-icon.png)) hopefully picks up the correct icon.
Let's see if it worked. In case of bitrot, try to apply on top of the patch in 	bug 681735.

I've also added the min-height for pinstripe so that anybody can try it.

I tested this only on TB with gnomestripe.
Attachment #626901 - Attachment is obsolete: true
Attachment #627766 - Flags: ui-review?(bwinton)
Attachment #627766 - Flags: review?(philip.chee)
(Assignee)

Updated

5 years ago
Attachment #627766 - Flags: review?(philipp)
Comment on attachment 627766 [details] [diff] [review]
patch with new icons

This definitely makes the situation on OSX better, so ui-r=me!

Thanks,
Blake.
Attachment #627766 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 627766 [details] [diff] [review]
patch with new icons


>diff --git a/calendar/base/themes/gnomestripe/images/cal-icon32.png b/calendar/base/themes/gnomestripe/images/cal-icon32.png
>index 3ddd72aa88b3b0903df3b3522112502cf098604b..1ecfba92bf18072575dc352417ab84eeb141b70f

>diff --git a/calendar/base/themes/winstripe/images/cal-icon32.png b/calendar/base/themes/winstripe/images/cal-icon32.png
>index 3ddd72aa88b3b0903df3b3522112502cf098604b..6601ed9fa26066d04fc78d9f732c83800470b869

Why is cal-icon32 being changed?



Regarding cal-icon.png, I would prefer it being named cal-icon24.png.

Aside from that, r=philipp for the calendar parts.
Attachment #627766 - Flags: review?(philipp) → review+
(Assignee)

Comment 14

5 years ago
(In reply to Philipp Kewisch [:Fallen] (away until May 28th) from comment #13)
> >diff --git a/calendar/base/themes/winstripe/images/cal-icon32.png b/calendar/base/themes/winstripe/images/cal-icon32.png
> >index 3ddd72aa88b3b0903df3b3522112502cf098604b..6601ed9fa26066d04fc78d9f732c83800470b869
> 
> Why is cal-icon32 being changed?

Andreas?

> Regarding cal-icon.png, I would prefer it being named cal-icon24.png.

I can fix that.
(Assignee)

Comment 15

5 years ago
It seems andreas optimized the icon32 (it is smaller filesize now) and is less blue to be in line with the other icons in the theme.
(Assignee)

Comment 16

5 years ago
Created attachment 631739 [details] [diff] [review]
patch v2

Unbitrotted after licence changes. Renamed the cal-icon to cal-icon24. I like the change to cal-icon32, it is now white, and fits nicely with the other calendar icons and also other icons on the Account central page. At least on gnomestripe.
Philip, please see if Seamonkey works OK, preferably on more platforms.
Attachment #627766 - Attachment is obsolete: true
Attachment #627766 - Flags: review?(philip.chee)
Attachment #631739 - Flags: review?(philip.chee)

Comment 17

5 years ago
Comment on attachment 631739 [details] [diff] [review]
patch v2

Certainly improves the look in SeaMonkey. Tested on Windows 7 both Classic and Modern themes.
Attachment #631739 - Flags: review?(philip.chee) → review+

Comment 18

5 years ago
Comment on attachment 631739 [details] [diff] [review]
patch v2

Certainly improves the look in SeaMonkey. Tested on Windows 7 both Classic and Modern themes.
(Assignee)

Comment 19

5 years ago
Comment on attachment 631739 [details] [diff] [review]
patch v2

Mconley, I am not sure we need any more reviews here.
Attachment #631739 - Flags: review?(mconley)
(In reply to :aceman from comment #15)
> It seems andreas optimized the icon32 (it is smaller filesize now) and is
> less blue to be in line with the other icons in the theme.

Please note that changing cal-icon32 also changes the addon icon. Just something to keep in mind while changing it.
Comment on attachment 631739 [details] [diff] [review]
patch v2

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

Didn't test it, but the code looks good. I like that our CSS got DRY'd up in the process, so kudos.
Attachment #631739 - Flags: review?(mconley) → review+
(Assignee)

Comment 22

5 years ago
(In reply to Philipp Kewisch [:Fallen] from comment #20)
> (In reply to :aceman from comment #15)
> > It seems andreas optimized the icon32 (it is smaller filesize now) and is
> > less blue to be in line with the other icons in the theme.
> 
> Please note that changing cal-icon32 also changes the addon icon. Just
> something to keep in mind while changing it.

Is this a problem in any way? Who can decide that?
(Assignee)

Updated

5 years ago
Attachment #631739 - Flags: review?(philipp)
Comment on attachment 631739 [details] [diff] [review]
patch v2

Ok, lets do it. Sorry for the delay :)
Attachment #631739 - Flags: review?(philipp) → review+
(Assignee)

Updated

5 years ago
Keywords: uiwanted → checkin-needed
https://hg.mozilla.org/comm-central/rev/433268decbc6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.