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

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1457781 removed the Warning.png we use on some places.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: warning.patch

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"] {}?

Comment 2

[Jorg K (CEST = GMT+2)]

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?

Comment 3

[Richard Marti (:Paenglab)]

I have no test case. Almost never use activity.

Comment 4

[[:MakeMyDay]]

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

Comment 5

[Richard Marti (:Paenglab)]

Attachment: warning.patch

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.

Comment 6

[[:MakeMyDay]]

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?

Comment 7

[Richard Marti (:Paenglab)]

(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

[Jorg K (CEST = GMT+2)]

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?

Comment 9

[Richard Marti (:Paenglab)]

(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 [e-mail] in email wizard, then you see the warning icon.

Comment 10

[Jorg K (CEST = GMT+2)]

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

Comment 11

[Jorg K (CEST = GMT+2)]

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

Comment 12

[Jorg K (CEST = GMT+2)]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b027417cfd9f
Fix the warning icons after removal of Warning.png in bug 1457781. r=jorgk,MakeMyDay

Comment 14

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 15

[Jorg K (CEST = GMT+2)]

... if we/you consider ...