Closed Bug 956589 Opened 10 years ago Closed 10 years ago

Port Bug 954653 - Redesign buddy tooltips

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 2 obsolete files)

This bug is to follow the IB tooltip redesign form bug 954653.
Attached patch patch (obsolete) — Splinter Review
This patch needs bug 956579 applied first.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8355914 - Flags: review?(aleth)
(In reply to Richard Marti (:Paenglab) from comment #1)
> This patch needs bug 956579 applied first.

After both these patches are applied, how forked is buddytooltip.xml/imbuddytooltip.xml, really? (Other than the path)
(In reply to aleth from comment #2)
> 
> After both these patches are applied, how forked is
> buddytooltip.xml/imbuddytooltip.xml, really? (Other than the path)

By porting this bug, I saw buddiesElt is missing in imbuddytooltip.xml:

@@ -267,17 +282,17 @@
 
          let buddies = this.contact.getBuddies();
          if (buddies.length <= 1)
            return true;
 
          let buddiesElt = document.getAnonymousElementByAttribute(this, "anonid",
                                                                   "buddies");
          let sep = document.createElement("separator");
-         sep.setAttribute("class", "groove");
+         sep.setAttribute("class", "groove tooltipSeparator");
          buddiesElt.appendChild(sep);
 
          for each (let buddy in buddies) {
            let buddyElt = document.createElement("buddy");
            buddiesElt.appendChild(buddyElt);
            buddyElt.build(buddy, this);
          }
Comment on attachment 8355914 [details] [diff] [review]
patch

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

It would probably have been better to merge patches in the order in which they landed in Instantbird: some of the changes of bug 956589 were included in the patch for bug 956579.

>By porting this bug, I saw buddiesElt is missing in imbuddytooltip.xml:

This is due to updateTooltipFromContact being much simpler for TB, as TB contacts don't ever contain multiple buddies at the moment.

>diff --git a/chat/themes/buddytooltip.css b/chat/themes/buddytooltip.css
>new file mode 100644
>--- /dev/null
>+++ b/chat/themes/buddytooltip.css

This new file should live in mail/components/im/content. While we should certainly try to unfork the tooltip code asap, until that is done, this file shouldn't be in /chat.

Looks good otherwise!
Attachment #8355914 - Flags: review?(aleth) → review-
Depends on: 956579
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to aleth from comment #4)
> Comment on attachment 8355914 [details] [diff] [review]
> patch
> 
> Review of attachment 8355914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would probably have been better to merge patches in the order in which
> they landed in Instantbird: some of the changes of bug 956589 were included
> in the patch for bug 956579.

Yeah, I saw this also after putting the patch for bug 956579 for review.

> >diff --git a/chat/themes/buddytooltip.css b/chat/themes/buddytooltip.css
> >new file mode 100644
> >--- /dev/null
> >+++ b/chat/themes/buddytooltip.css
> 
> This new file should live in mail/components/im/content. While we should
> certainly try to unfork the tooltip code asap, until that is done, this file
> shouldn't be in /chat.

Moved.
Attachment #8355914 - Attachment is obsolete: true
Attachment #8359304 - Flags: review?(aleth)
Comment on attachment 8359304 [details] [diff] [review]
patch v2

>diff --git a/mail/components/im/content/imBuddytooltip.css b/mail/components/im/content/imBuddytooltip.css
>new file mode 100644
>--- /dev/null
>+++ b/mail/components/im/content/imBuddytooltip.css

nit: This should be imbuddytooltip.css to be consistent with imbuddytooltip.xml.
Attachment #8359304 - Flags: review?(aleth) → review-
Attached patch patch v3Splinter Review
Now it's inconsistent with chrome://messenger/skin/imBuddytooltip.css ;)
Attachment #8359304 - Attachment is obsolete: true
Attachment #8359704 - Flags: review?(aleth)
Comment on attachment 8359704 [details] [diff] [review]
patch v3

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

(In reply to Richard Marti (:Paenglab) from comment #7)
> Created attachment 8359704 [details] [diff] [review]
> patch v3
> 
> Now it's inconsistent with chrome://messenger/skin/imBuddytooltip.css ;)

This is true. Yet another reason to unfork this code ;)

Thanks!
Attachment #8359704 - Flags: review?(aleth) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/76d9545cff20
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Depends on: 962035
You need to log in before you can comment on or make changes to this bug.