Modern Update: global/notification.css

RESOLVED FIXED in seamonkey2.0

Status

SeaMonkey
Themes
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

({fixed-seamonkey2.0, modern})

Trunk
seamonkey2.0
fixed-seamonkey2.0, modern
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Splitting off notification.css from Bug 465924.
(Assignee)

Comment 1

8 years ago
Created attachment 399723 [details] [diff] [review]
Patch v1.0

>  .messageImage {
>    width: 16px;
>    height: 16px;
>    margin-top: 0px;
>    margin-bottom: 0px;
> -  -moz-margin-start: 6px;
> +  -moz-margin-start: 5px;

I think 5px looks better.

> diff --git a/suite/themes/modern/jar.mn b/suite/themes/modern/jar.mn
> +  skin/modern/global/icons/question-16.png                         (global/icons/question-16.png)

I'm including question-16.png because it is already referenced elsewhere in suite.

There are some things from Kudens notification.css that I didn't include because I didn't understand why they were there e.g.

border-colour and -moz-border-radius on the .messageCloseButton.

I didn't use Kudens background colours for the notifications.

> notification {
>   color: #000000;
>   background-color: #FFFFE0;
> }
> 
> notification[type="info"] {
>   background-color: #C7D0D9;
> }
> 
> notification[type="critical"] {
>   background-color: #FFCCCC;
> }

Winstripe has this rule but I think it makes things look worse so I didn't include it:

> .messageCloseButton > .toolbarbutton-icon {
>   -moz-margin-end: 5px;
> }
Attachment #399723 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED

Comment 2

8 years ago
Comment on attachment 399723 [details] [diff] [review]
Patch v1.0

>GIT binary patch
Haven't looked at the images yet...

>+  color: #000000;
>   background-color: #E8DB99;
>-  color: #000000;
background-colo(u)r first throughout please.

>+notification:not([type="info"]) > .notification-inner {
.notification-inner inherits the type, so you can check it directly. Also, the type can only be info, warning and critical, and you're already overriding it for critical, so this is only effective for warning. It might make more sense to default to info background colour and override for warning too.

>+  border-color: #E8DB99 !important;
Why !important?

>+notification[type="info"] .messageImage {
Sadly .messageImage does not inherit type :-(
.notification-inner[type="info"] > hbox > .messageImage

>+  margin-top: 0;
>+  margin-bottom: 0;
Nit: 0px;

Comment 3

8 years ago
Comment on attachment 399723 [details] [diff] [review]
Patch v1.0

> .messageImage {
>   width: 16px;
>   height: 16px;
>   margin-top: 0px;
>   margin-bottom: 0px;
I noticed that the icons look very low with these margins. I'd have to try some other values e.g. 5px bottom to see how they look.

>+.messageText {
>+  margin-top: 0;
>+  margin-bottom: 0;
>+  -moz-margin-start: 5px;
>+  -moz-margin-end: 1px;
Ironically these margins *do* have to be important!

Comment 4

8 years ago
The alpha transparency was implemented really badly; the information icon would only work by chance because its background is the Modern default, but the other two icon's shadows won't work properly :-(
Created attachment 399811 [details]
High resolution icons (PSD file)

(In reply to comment #4)
> The alpha transparency was implemented really badly; the information icon would
> only work by chance because its background is the Modern default, but the other
> two icon's shadows won't work properly :-(

These look strange if there is no shadow...

Comment 6

8 years ago
My fault, I hadn't realised that the shadow was quite so blue.
Created attachment 399827 [details]
Color sample
(Assignee)

Comment 8

8 years ago
Created attachment 400022 [details] [diff] [review]
Patch v1.1

> >+  color: #000000;
> >   background-color: #E8DB99;
> >-  color: #000000;
> background-colo(u)r first throughout please.
Fixed.

> >+notification:not([type="info"]) > .notification-inner {
> .notification-inner inherits the type, so you can check it directly. Also, the
> type can only be info, warning and critical, and you're already overriding it
> for critical, so this is only effective for warning. It might make more sense
> to default to info background colour and override for warning too.
Fixed.

> >+  border-color: #E8DB99 !important;
> Why !important?

Apparently there is also a rule for the outset class coming from global.css which sets the border colour and which gets applied after the rules in notification.css.

> >+notification[type="info"] .messageImage {
> Sadly .messageImage does not inherit type :-(
> .notification-inner[type="info"] > hbox > .messageImage
Fixed.

> >+  margin-top: 0;
> >+  margin-bottom: 0;
> Nit: 0px;
Fixed.

> > .messageImage {
> >   width: 16px;
> >   height: 16px;
> >   margin-top: 0px;
> >   margin-bottom: 0px;
> I noticed that the icons look very low with these margins. I'd have to try some
> other values e.g. 5px bottom to see how they look.

I found that 5px was too much. 3px seems right to me but this is awfully subjective.

> >+.messageText {
> >+  margin-top: 0;
> >+  margin-bottom: 0;
> >+  -moz-margin-start: 5px;
> >+  -moz-margin-end: 1px;
> Ironically these margins *do* have to be important!
Fixed.
Attachment #399723 - Attachment is obsolete: true
Attachment #400022 - Flags: review?(neil)
Attachment #399723 - Flags: review?(neil)

Comment 9

8 years ago
Comment on attachment 400022 [details] [diff] [review]
Patch v1.1

> notification {
>   background-color: #E8DB99
>   color: #000000;
> }
> 
> notification[type="info"] {
>   background-color: #C7D0D9;
> }
[The idea was that we swap the background colours around so that the "default"
 type is "info" and then type="warning" overrides the background and border.]
Attachment #400022 - Flags: review?(neil) → review+

Updated

8 years ago
Attachment #400022 - Flags: approval-seamonkey2.0+

Comment 10

8 years ago
Comment on attachment 400022 [details] [diff] [review]
Patch v1.1

Setting a+ so this can go in without further ado.
(Assignee)

Comment 11

8 years ago
Created attachment 400361 [details] [diff] [review]
Patch v1.2
[Checkin: Comment 12]

Carrying forward r=Neil

> (From update of attachment 400022 [details] [diff] [review])
>> notification {
>>   background-color: #E8DB99
>>   color: #000000;
>> }
>> 
>> notification[type="info"] {
>>   background-color: #C7D0D9;
>> }
> [The idea was that we swap the background colours around so that the "default"
>  type is "info" and then type="warning" overrides the background and border.]
Oops. Fixed.
Attachment #400022 - Attachment is obsolete: true
Attachment #400361 - Flags: superreview?(neil)
Attachment #400361 - Flags: review+

Updated

8 years ago
Attachment #400361 - Flags: superreview?(neil) → superreview+
(Assignee)

Updated

8 years ago
Attachment #400361 - Flags: approval-seamonkey2.0?

Updated

8 years ago
Attachment #400361 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
(Assignee)

Updated

8 years ago
Attachment #400361 - Attachment description: Patch v1.2 r=Neil → [for checkin] Patch v1.2 r=Neil
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Comment on attachment 400361 [details] [diff] [review]
Patch v1.2
[Checkin: Comment 12]


http://hg.mozilla.org/comm-central/rev/e80058ce2515
Attachment #400361 - Attachment description: [for checkin] Patch v1.2 r=Neil → Patch v1.2 [Checkin: Comment 12]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed → fixed-seamonkey2.0
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
You need to log in before you can comment on or make changes to this bug.