Fix the warning icons after removal of Warning.png in bug 1457781

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 64.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

9 months ago
Bug 1457781 removed the Warning.png we use on some places.
Assignee

Comment 1

9 months ago
Posted patch warning.patch (obsolete) — Splinter Review
I copied the image to activity on Windows to still be consistent with the other icons in activity.

MakeMyDay, can you say me how I can test the #reminder-notifications > notification[type="warning"] {}?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9009350 - Flags: review?(makemyday)
Attachment #9009350 - Flags: review?(jorgk)

Comment 2

9 months ago
Comment on attachment 9009350 [details] [diff] [review]
warning.patch

Looks OK on visual inspection. I can't recall ever having seen a warning in the Activity Manager. Do you have a test case?
Attachment #9009350 - Flags: review?(jorgk) → review+
Assignee

Comment 3

9 months ago
I have no test case. Almost never use activity.

Comment 4

9 months ago
I'm wondering why you need to add this additional styles here, since this is a notificationbox. Shouldn't be everything you added being used by it by default already?

Regarding your question: Provider for Google and the wcap provider have hard coded values for this (5/1). For any other type of calendar, you can define a limit per calendar by adding a custom integer preference

calendar.registry.calendar-guid.capabilities.alarms.maxCount

Replace |calendar-guid| by the guid of the calendar you want to use to test. Then create a new event and add a custom reminder. The opened reminder dialog window now should show the warning at the top. Don't forget to remove the pref afterwards.

If you just want to cross check the notification, you could also just add |cond = true;| right before for [1] to make it always visible.

[1] https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-reminder.js#186
Assignee

Comment 5

9 months ago
Posted patch warning.patchSplinter Review
MakeMyDay, thanks for the guide. The notification bar needed more tweaks as the notification-inner no more exists. I made the icon 32px * 32px like it was before in the notification to make more attention, OK?

Jörg, bug 1457784 becomes some activity now, so I decided to fix the warning-16.png here already.
Attachment #9009350 - Attachment is obsolete: true
Attachment #9009350 - Flags: review?(makemyday)
Attachment #9009442 - Flags: review?(makemyday)
Attachment #9009442 - Flags: review?(jorgk)

Comment 6

9 months ago
Comment on attachment 9009442 [details] [diff] [review]
warning.patch

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

Thanks. Do we need changes for our uses of notificationbox as well?
Attachment #9009442 - Flags: review?(makemyday) → review+
Assignee

Comment 7

9 months ago
(In reply to [:MakeMyDay] from comment #6)
> Comment on attachment 9009442 [details] [diff] [review]
> warning.patch
> 
> Review of attachment 9009442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. Do we need changes for our uses of notificationbox as well?

I don't think. notification-inner was only used in this place and suite. And all other notification rules should work correctly.

Comment 8

9 months ago
Comment on attachment 9009442 [details] [diff] [review]
warning.patch

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

::: mail/themes/linux/mail/accountCreation.css
@@ +215,5 @@
>  }
>  
>  #status_area[status=error] #status_img_before {
> +  background: url("chrome://global/skin/icons/warning.svg") no-repeat;
> +  background-position-y: 3px;

Why is that necessary?
Assignee

Comment 9

9 months ago
(In reply to Jorg K (GMT+2) from comment #8)
> >  #status_area[status=error] #status_img_before {
> > +  background: url("chrome://global/skin/icons/warning.svg") no-repeat;
> > +  background-position-y: 3px;
> 
> Why is that necessary?

The icon is too high compared to the text. This is already with the actual icon. I thought moving it a bit to the bottom looks better. You can try to test a not existing email like a@ee.rr in email wizard, then you see the warning icon.

Comment 10

9 months ago
Hmm, too low now. I'd move it 2px up again.

Comment 11

9 months ago
Yep, background-position-y: 1px; looks good. Shall I do that?

Comment 12

9 months ago
Comment on attachment 9009442 [details] [diff] [review]
warning.patch

I can't find a way to test the Activity Manager, even when trying to modify code to force that icon. The icon looks good though.

As I said, in the Account Manager the offset should only be 1px. I can do that, no new patch required.
Attachment #9009442 - Flags: review?(jorgk) → review+

Comment 13

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b027417cfd9f
Fix the warning icons after removal of Warning.png in bug 1457781. r=jorgk,MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED

Comment 14

9 months ago
Landed with 1px offset. Like this, on Windows, the icon base goes 2px below the baseline of the text and 3px above the top of the text. Since the heavy bottom part is, well, bigger, 2px below and 3px above is perfect visually if the consider the overshooting area ;-)
Target Milestone: --- → Thunderbird 64.0

Comment 15

9 months ago
... if we/you consider ...
You need to log in before you can comment on or make changes to this bug.