Closed Bug 1445916 Opened 3 years ago Closed 3 years ago

Use svg icons headerPane's emailButton and in edit contacts panel

Categories

(Thunderbird :: Theme, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird60 fixed, thunderbird61 fixed)

RESOLVED FIXED
Thunderbird 61.0
Tracking Status
thunderbird60 --- fixed
thunderbird61 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(2 files, 1 obsolete file)

Like the FX bug 1441727, we should also use a SVG icon for the star in the address edit popup (the popup that opens when you click on the star on the right of the address in message header.
Attached patch starIcon.patch (obsolete) — Splinter Review
Additionally to the icon exchange, I aligned the star better to the address in message header. Also the header in the popup is better aligned. The buttons end now aligned with the menulists too.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8959110 - Flags: review?(jorgk)
Jörg, you can also test with my crazy colours WE-test-theme to see that it's coloured like it does on FX bookmark popup.
Comment on attachment 8959110 [details] [diff] [review]
starIcon.patch

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

Works for me.

(In reply to Richard Marti (:Paenglab) from comment #2)
> Jörg, you can also test with my crazy colours WE-test-theme to see that it's
> coloured like it does on FX bookmark popup.
I didn't test that. Which version of FF do I need? I only have the release version, but there, the bookmark star is blue like with this patch. Which theme are you referring to, the one from bug 1439134 or the one from bug 1435549 ("ugly colours"). It wouldn't be the one from bug 1443226.

I was wondering which comment conventions you use in CSS. Something like
/* this is a comment that starts on one line
   and ends on the next */
would not pass review.

::: mail/themes/shared/mail/messageHeader.css
@@ +141,5 @@
> +  color: HighlightText;
> +  background-color: Highlight;
> +}
> +
> +mail-emailaddress[selected="true"] > .emailDisplayButton{

space before {

@@ +144,5 @@
> +
> +mail-emailaddress[selected="true"] > .emailDisplayButton{
> +  /* when an email address context menu is selected,
> +    make sure that the email bubble stays displayed, and
> +    tweak the bottom to blend in more w/ the menu */

Ugly comment. OK, pre-existing, was moved.

@@ +161,5 @@
>  /* Because there's no star for newsgroups like there is for email addresses,
>   * pushing it back the same number of pixels as emailSeparator causes
>   * the comma to be drawn too close to the last letter, which looks bad, so
>   * using a separate rule here.
>   */

Here's a better looking comment.
Attachment #8959110 - Flags: review?(jorgk) → review+
Attached patch starIcon.patchSplinter Review
(In reply to Jorg K (GMT+1) from comment #3)
> Comment on attachment 8959110 [details] [diff] [review]
> starIcon.patch
> 
> Review of attachment 8959110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Works for me.
> 
> (In reply to Richard Marti (:Paenglab) from comment #2)
> > Jörg, you can also test with my crazy colours WE-test-theme to see that it's
> > coloured like it does on FX bookmark popup.
> I didn't test that. Which version of FF do I need? I only have the release
> version, but there, the bookmark star is blue like with this patch. Which
> theme are you referring to, the one from bug 1439134 or the one from bug
> 1435549 ("ugly colours"). It wouldn't be the one from bug 1443226.

FX 60. Sorry, none of the extensions I uploaded there. It was a later m-c change of colouring arrow popups I have in the extension which needed no change in TB. I'll attach it therefore you can test it if you like.

> I was wondering which comment conventions you use in CSS. Something like
> /* this is a comment that starts on one line
>    and ends on the next */
> would not pass review.

This is a standard CSS comment. Everything after a /* is commented out until a */ appears. It doesn't end at line end.

> ::: mail/themes/shared/mail/messageHeader.css
> @@ +141,5 @@
> > +  color: HighlightText;
> > +  background-color: Highlight;
> > +}
> > +
> > +mail-emailaddress[selected="true"] > .emailDisplayButton{
> 
> space before {

Fixed.

> @@ +144,5 @@
> > +
> > +mail-emailaddress[selected="true"] > .emailDisplayButton{
> > +  /* when an email address context menu is selected,
> > +    make sure that the email bubble stays displayed, and
> > +    tweak the bottom to blend in more w/ the menu */
> 
> Ugly comment. OK, pre-existing, was moved.

I now added the leading * to be more consistent with the following comment and exchanged the "bubble" to "menupopup" to be more precise.
Attachment #8959110 - Attachment is obsolete: true
Attachment #8959443 - Flags: review+
Attached file test-webext-theme.xpi
newest "ugly colours" test theme, if you want test it.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0ab0740d6969
Use svg icons headerPane's email button and in edit contacts panel. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
Comment on attachment 8959443 [details] [diff] [review]
starIcon.patch

Can we have this uplifted for FX parity?
Attachment #8959443 - Flags: approval-comm-beta?
Attachment #8959443 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.