885740 - Account central 2x icons for mac layout

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[alta88]

a css adjustment is needed for 2x mac icons, after Bug 529131.

richard could you help with this?

Comment 1

[Richard Marti (:Paenglab)]

To fix this problem you need only change the background-size: 32px 32px; to background-size: 16px 16px; on your newly added icons.

The rule for #CreateAccountChat isn't needed when you are using the same icon as in LoDPI.

Comment 2

[alta88]

(In reply to Richard Marti [:Paenglab] from comment #1)
> To fix this problem you need only change the background-size: 32px 32px; to
> background-size: 16px 16px; on your newly added icons.
> 
> The rule for #CreateAccountChat isn't needed when you are using the same
> icon as in LoDPI.

thanks!  i'll submit the patch here in a bit..

Comment 3

[Richard Marti (:Paenglab)]

Attachment: chat-32.png

32px icon for #CreateAccountChat in HiDPI mode. I can't check if it's looking good because I have no Retina Mac.

Comment 4

[alta88]

hmm, since you have a new chat icon, maybe it's just easier if you could do the patch, with the patch in Bug 529131 applied?

thanks again.

Comment 5

[alta88]

oh, it also occurs to me that due to the feed icon revamp, there's a mismatch with the New Account feed icon and the server icon.  perhaps we could slip in a fix to make that icon match server rather than the new folder icon (consistent with the other account types).

Comment 6

[Richard Marti (:Paenglab)]

Attachment: patch

Patch setting the correct dimensions for HiDPI icons in Account Central. For Windows and Linux I added a new RSS icon. OS X is using a completely different and I leave it as it is.

Mark, please can you check on OS X if the icons are showing correctly?

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 767927 [details] [diff] [review]
patch

Looks good with the patch

Comment 8

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

Comment on attachment 767927 [details] [diff] [review]
patch

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

Yes, this looks much better. Thanks Richard!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/d3fcee9dc98d

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 767927 [details] [diff] [review]
patch

[Triage Comment]
We definitely want this on 24, thanks.

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/72f71f835916

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 767927 [details] [diff] [review]
patch

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

This patch contains chat/ changes and landed without appropriate review. None of the chat peers were cc'ed on the bug.

It looks to me like this bug was really intended to be a Mac-only Thunderbird-only front-end change, so the new file should have been placed in mail/themes/osx/mail/icons/.

::: chat/themes/jar.mn
@@ +19,5 @@
>  	skin/classic/chat/unknown.png
>  	skin/classic/chat/unknown-16.png
>  	skin/classic/chat/chat-16.png
>  	skin/classic/chat/chat-left-16.png
> +	skin/classic/chat/chat-32.png

You use this file only on Mac, so it shouldn't be packaged on all platforms.

::: mail/themes/osx/mail/accountCentral.css
@@ +229,4 @@
>    }
>  
>    #CreateAccountChat {
> +    background: url("chrome://chat/skin/chat-32.png") no-repeat;

This is really a 16px icon with a higher definition, so IMHO it should be named [e-mail] to be consistent with the other icons here.

Comment 13

[Florian Quèze [:florian]]

Attachment: chat/ cleanup

I think this is what was intended. If we had discovered earlier I would have just backed out the patch.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 830993 [details] [diff] [review]
chat/ cleanup

Note: this patch is completely untested.

Comment 15

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)

> It looks to me like this bug was really intended to be a Mac-only
> Thunderbird-only front-end change, so the new file should have been placed
> in mail/themes/osx/mail/icons/.

But if I misunderstood and you are really trying to fix chat/ files to look better on retina Macs, we are happy to work with you on it (likely in a different bug though, as this one really seems Thunderbird specific).

Comment 16

[Florian Quèze [:florian]]

Quote from the minutes of last week's Thunderbird status meeting:
"Please request review from Florian (:florian) or Patrick (:clokep) on all changes that touch chat/! If you know you do not need our review (e.g. jcranmer's mass build infrastructure changes are fine), CCing us on the bug would still be appreciated!"

I'm adding a needinfo flag on both Mike and Mark to ensure you have seen this, as you both reviewed/approved the patch without sending us a heads-up.

Comment 17

[Mark Banner (:standard8)]

Note, if this patch is expected to go forward to branches, please file it on a new bug, to make it easier to track.

Comment 18

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #17)
> Note, if this patch is expected to go forward to branches, please file it on
> a new bug, to make it easier to track.

We don't need it on branches, but I would like it to land on central before bug 920801.

Comment 19

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

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> Quote from the minutes of last week's Thunderbird status meeting:
> "Please request review from Florian (:florian) or Patrick (:clokep) on all
> changes that touch chat/! If you know you do not need our review (e.g.
> jcranmer's mass build infrastructure changes are fine), CCing us on the bug
> would still be appreciated!"
> 
> I'm adding a needinfo flag on both Mike and Mark to ensure you have seen
> this, as you both reviewed/approved the patch without sending us a heads-up.

Yikes! My apologies - I should have been paying more attention. :(

Comment 20

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

Comment on attachment 830993 [details] [diff] [review]
chat/ cleanup

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

You'll have to get rid of the chat-32.png reference in chat/themes/jar.mn, but other than that, this works fine. Thanks Florian!

Comment 21

[Florian Quèze [:florian]]

https://hg.mozilla.org/comm-central/rev/39f6af451ff3