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

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

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

Comment 1

[:aceman]

Attachment: 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

Comment 2

[:aceman]

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

[Philip Chee]

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

Comment 4

[Andreas Nilsson (:andreasn)]

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

Comment 5

[Andreas Nilsson (:andreasn)]

Attachment: 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).

Comment 6

[:aceman]

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?

Comment 7

[:aceman]

Andreas, while you are in icon creation mode, I think you have not yet responded to bug 727598 :)

Comment 8

[Andreas Nilsson (:andreasn)]

(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

[Ian Neal]

Have these icons been run through the likes of OptiPNG?

Comment 10

[:aceman]

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

Comment 11

[:aceman]

Attachment: 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.

Comment 12

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

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.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 14

[:aceman]

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

Comment 15

[:aceman]

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.

Comment 16

[:aceman]

Attachment: 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.

Comment 17

[Philip Chee]

Comment on attachment 631739 [details] [diff] [review]
patch v2

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

Comment 18

[Philip Chee]

Comment on attachment 631739 [details] [diff] [review]
patch v2

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

Comment 19

[:aceman]

Comment on attachment 631739 [details] [diff] [review]
patch v2

Mconley, I am not sure we need any more reviews here.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 22

[:aceman]

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

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 631739 [details] [diff] [review]
patch v2

Ok, lets do it. Sorry for the delay :)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/433268decbc6