Closed Bug 954557 Opened 10 years ago Closed 10 years ago

Some minor conversation CSS improvements

Categories

(Instantbird Graveyard :: Conversation, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1124 at 2011-10-30 14:50:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1124 as attmnt 955 at 2011-10-30 14:50:00 UTC ***

Very minor ;) As discussed on IRC.
Attachment #8352697 - Flags: review?(florian)
Comment on attachment 8352697 [details] [diff] [review]
Patch

*** Original change on bio 1124 attmnt 955 at 2011-10-30 16:04:25 UTC ***

>--- conversation.xml	2011-10-28 21:32:53.000000000 +0200
>+++ conversation.xml	2011-10-28 02:13:31.000000000 +0200

>               <xul:browser anonid="browser" type="content-conversation" flex="1"
>-                           xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autoscrollpopup"/>
>+                           xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autoscrollpopup,chat"/>

Why is the chat attribute needed on this tag?

>--- conversation.css  2011-10-30 15:45:05.000000000 +0100
>+++ conversation.css  2011-10-30 15:45:11.000000000 +0100
>@@ -269,20 +269,20 @@
> %endif
> }
>
>-conversation[chat="true"] .conv-messages {
>+.conv-messages[chat] {
> %ifndef XP_MACOSX
>   border-right: solid 1px GrayText;
> %endif
> }

While you are cleaning up/optimizing, you could as well move the ifdef out so that the selector only exists in the file when there's going to be a rule for it ;).
Attachment #8352697 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1124 as attmnt 956 at 2011-10-30 16:14:00 UTC ***

Corrected patch.

Also modified the Mac-only section for consistency, but couldn't test this.
Attachment #8352698 - Flags: review?(florian)
Comment on attachment 8352697 [details] [diff] [review]
Patch

*** Original change on bio 1124 attmnt 955 at 2011-10-30 16:14:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352697 - Attachment is obsolete: true
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1124 as attmnt 957 at 2011-10-30 16:20:00 UTC ***

Added missing %endif.
Attachment #8352699 - Flags: review?(florian)
Comment on attachment 8352698 [details] [diff] [review]
Patch

*** Original change on bio 1124 attmnt 956 at 2011-10-30 16:20:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352698 - Attachment is obsolete: true
Attachment #8352698 - Flags: review?(florian)
Attached patch PatchSplinter Review
*** Original post on bio 1124 as attmnt 958 at 2011-10-30 16:27:00 UTC ***

This is getting embarrassing...
Attachment #8352700 - Flags: review?(florian)
Comment on attachment 8352699 [details] [diff] [review]
Patch

*** Original change on bio 1124 attmnt 957 at 2011-10-30 16:27:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352699 - Attachment is obsolete: true
Attachment #8352699 - Flags: review?(florian)
Comment on attachment 8352700 [details] [diff] [review]
Patch

*** Original change on bio 1124 attmnt 958 at 2011-10-30 16:40:15 UTC ***

Looks good :). I'll try this on Mac before pushing the changes.
Attachment #8352700 - Flags: review?(florian) → review+
*** Original post on bio 1124 at 2011-11-09 23:14:52 UTC ***

Comment on attachment 8352700 [details] [diff] [review] (bio-attmnt 958)
Patch

>--- conversation.css  2011-10-30 15:45:35.000000000 +0100
>+++ conversation.css  2011-10-30 17:18:21.000000000 +0100

>@@ -314,7 +314,7 @@
>   border-style:none;

I've just noticed that a space was missing here between : and none. I've added it (even though it's not directly related to your performance improvements, but as we are cleaning up... :)).

>-conversation:not([chat="true"]) .conv-chat {
>+.conv-chat:not([chat]) {
>   display: none;
> }

The good place for this rule is in instantbird/content/instantbird.css rather than this theme file. I've moved it during my testing. Just for information: we usually put the rules that decide what will be shown (typically -moz-binding and display: none rules) in content stylesheets, and rules that decide how it will be shown in themes stylesheets.

Seems to work well, thanks for cleaning this up!
*** Original post on bio 1124 at 2011-11-10 00:52:26 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/8430992e1544

Thanks! Sorry it took so long for us to get around to this!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.