Last Comment Bug 758306 - make the "Add new calendar" icon the same height of 24px as the other icons in the Account central page.
: make the "Add new calendar" icon the same height of 24px as the other icons i...
Status: RESOLVED FIXED
: polish
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 17.0
Assigned To: :aceman
:
Mentors:
Depends on: 681735
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 11:33 PDT by :aceman
Modified: 2012-07-22 07:35 PDT (History)
9 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (6.09 KB, patch)
2012-05-24 11:42 PDT, :aceman
acelists: ui‑review-
Details | Diff | Splinter Review
24x24 icons for gnomestripe and winstripe (6.47 KB, patch)
2012-05-28 06:59 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
patch with new icons (13.49 KB, patch)
2012-05-28 12:50 PDT, :aceman
philipp: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v2 (13.69 KB, patch)
2012-06-10 05:40 PDT, :aceman
philip.chee: review+
mconley: review+
philipp: review+
Details | Diff | Splinter Review

Description :aceman 2012-05-24 11:33:19 PDT
+++ 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 2012-05-24 11:42:25 PDT
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
Comment 2 :aceman 2012-05-24 11:43:51 PDT
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 2012-05-24 18:03:43 PDT
> 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) 2012-05-28 03:44:23 PDT
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) 2012-05-28 06:59:55 PDT
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).
Comment 6 :aceman 2012-05-28 07:08:06 PDT
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 2012-05-28 07:28:13 PDT
Andreas, while you are in icon creation mode, I think you have not yet responded to bug 727598 :)
Comment 8 Andreas Nilsson (:andreasn) 2012-05-28 07:31:39 PDT
(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 2012-05-28 09:59:34 PDT
Have these icons been run through the likes of OptiPNG?
Comment 10 :aceman 2012-05-28 12:17:07 PDT
(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 2012-05-28 12:50:56 PDT
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.
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-05-31 07:16:32 PDT
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] 2012-06-07 06:19:46 PDT
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 2012-06-10 05:04:10 PDT
(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 2012-06-10 05:14:46 PDT
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 2012-06-10 05:40:09 PDT
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.
Comment 17 Philip Chee 2012-06-11 08:43:17 PDT
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 2012-06-11 08:50:14 PDT
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 2012-06-11 12:55:36 PDT
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] 2012-06-13 15:18:02 PDT
(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) - (needinfo me!) 2012-06-18 07:24:15 PDT
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 2012-06-18 08:10:33 PDT
(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] 2012-07-22 00:22:12 PDT
Comment on attachment 631739 [details] [diff] [review]
patch v2

Ok, lets do it. Sorry for the delay :)
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-07-22 07:35:27 PDT
https://hg.mozilla.org/comm-central/rev/433268decbc6

Note You need to log in before you can comment on or make changes to this bug.