Last Comment Bug 795989 - cleanup of folderpane icons
: cleanup of folderpane icons
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: Thunderbird 20.0
Assigned To: Andreas Nilsson (:andreasn)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-01 10:25 PDT by Andreas Nilsson (:andreasn)
Modified: 2012-11-29 13:50 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
folder pane icons (wip) (27.79 KB, patch)
2012-10-01 10:27 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
updated patch (29.65 KB, text/plain)
2012-10-04 08:15 PDT, Andreas Nilsson (:andreasn)
no flags Details
folder pane icons (v3) (29.66 KB, patch)
2012-11-16 09:29 PST, Andreas Nilsson (:andreasn)
richard.marti: ui‑review-
Details | Diff | Splinter Review
folder pane icons (v4) (26.31 KB, patch)
2012-11-16 15:53 PST, Andreas Nilsson (:andreasn)
mconley: review+
richard.marti: ui‑review+
Details | Diff | Splinter Review

Description Andreas Nilsson (:andreasn) 2012-10-01 10:25:49 PDT
Qute and Gnomestripe themes make use of imagemaps for the folderpane icons. This requires less amount of files and also helps with performance slightly.
By doing this to pinstripe too, the code will be easier to read due to being more homogeneous between the different themes.
Comment 1 Andreas Nilsson (:andreasn) 2012-10-01 10:27:40 PDT
Created attachment 666604 [details] [diff] [review]
folder pane icons (wip)

Need to do some testing to make sure there are no regressions.
Comment 2 Andreas Nilsson (:andreasn) 2012-10-04 08:15:29 PDT
Created attachment 667987 [details]
updated patch
Comment 3 Richard Marti (:Paenglab) 2012-10-07 04:44:57 PDT
This looks good. I found one error in a -moz-image-region:

 /* ..... News Folders ..... */
 .tabmail-tab[type="folder"][ServerType="nntp"],
 treechildren::-moz-tree-image(folderNameCol, serverType-nntp) {
-  list-style-image: url("chrome://messenger/skin/icons/folder-newsgroup.png");
+  -moz-image-region: rect(0 176px 160px 0);

should be:
+  -moz-image-region: rect(0 176px 16px 160px);

Please can you look to finish this patch so it can land in the near future? I'm basing bug 781333 on this bug.
Comment 4 Andreas Nilsson (:andreasn) 2012-11-16 09:29:49 PST
Created attachment 682502 [details] [diff] [review]
folder pane icons (v3)

Third version of the patch, fixing the error Richard pointed out.
Comment 5 Andreas Nilsson (:andreasn) 2012-11-16 09:32:29 PST
Comment on attachment 682502 [details] [diff] [review]
folder pane icons (v3)

Setting up for review
Comment 6 Richard Marti (:Paenglab) 2012-11-16 14:14:50 PST
Comment on attachment 682502 [details] [diff] [review]
folder pane icons (v3)

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

Why is gnomestripe/mail/icons/server.png in this patch? Have you something fixed here?

ui-r- because of the three comments.

Sorry to say the patch is looking good for review with not checking it deeply.

::: mail/themes/pinstripe/mail/folderPane.css
@@ +21,5 @@
>  
>  /* ..... News Folders ..... */
>  .tabmail-tab[type="folder"][ServerType="nntp"],
>  treechildren::-moz-tree-image(folderNameCol, serverType-nntp) {
> +  -moz-image-region: rect(0 176px 16px 160px);

Sorry, my bad this should be: rect(0 160px 16px 144px) to show the news folder and not the saved search folder.

@@ +146,1 @@
>    list-style-image: url("chrome://messenger/skin/icons/server-newsblog.png");

This rule should be removed. The icon is no more existent.

@@ +149,2 @@
>  /* .....  Newsgroup .....  */
>  .tabmail-tab[type="folder"][ServerType="nntp"] {

This rule needs also to be removed.
Comment 7 Andreas Nilsson (:andreasn) 2012-11-16 15:53:31 PST
Created attachment 682665 [details] [diff] [review]
folder pane icons (v4)

>Why is gnomestripe/mail/icons/server.png in this patch? Have you something fixed here?

Reverted to the old one. Will fix this graphics in a separate patch.


>::: mail/themes/pinstripe/mail/folderPane.css
>@@ +21,5 @@
>>  
>>  /* ..... News Folders ..... */
>>  .tabmail-tab[type="folder"][ServerType="nntp"],
>>  treechildren::-moz-tree-image(folderNameCol, serverType-nntp) {
>> +  -moz-image-region: rect(0 176px 16px 160px);

>Sorry, my bad this should be: rect(0 160px 16px 144px) to show the news folder and not the saved search folder.

Fixed


>@@ +146,1 @@
>>    list-style-image: url("chrome://messenger/skin/icons/server newsblog.png");

>This rule should be removed. The icon is no more existent.

Fixed.


>@@ +149,2 @@
>>  /* .....  Newsgroup .....  */
>>  .tabmail-tab[type="folder"][ServerType="nntp"] {
>This rule needs also to be removed.

Fixed
Comment 8 Richard Marti (:Paenglab) 2012-11-17 01:37:46 PST
Comment on attachment 682665 [details] [diff] [review]
folder pane icons (v4)

Looks good now.
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-11-21 19:31:21 PST
Comment on attachment 682665 [details] [diff] [review]
folder pane icons (v4)

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

Looks good! Thanks Andreas!
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-11-23 10:02:05 PST
https://hg.mozilla.org/comm-central/rev/d04bed4359e3

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