[de-xbl] convert group binding to <richlistitem is="chat-group">

RESOLVED FIXED in Thunderbird 68.0

Status

defect
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: mkmelin, Assigned: khushil324)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 68.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Comment 1

a month ago
Attachment #9057474 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 2

a month ago
Comment on attachment 9057474 [details] [diff] [review]
Bug-1538550_richlistitem_chat-group.patch

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

Please expand the commit message a bit

::: mail/components/im/content/chat-group.js
@@ +8,4 @@
>  
> +/**
> +  * The MozChatGroup widget displays chat group name and behave as a dropdown
> +  * menu for chat contacts.

dropdown menu? It's a twisty for expansion I think. I guess this is e.g the "Offline Contacts" section? If so, please improve the description with an example.

@@ +27,5 @@
> +    this.setAttribute("collapsed", "true");
> +
> +    this.appendChild(MozXULElement.parseXULToFragment(`
> +      <image class="twisty"></image>
> +      <label flex="1" crop="end" inherits="value=name"></label>

should remove the inherits here

maybe just build this manually intstead of using parseXULToFragement, since this is tiny and parsing will be slower

@@ +39,5 @@
>  
> +    this.addEventListener("click", (event) => {
> +      // Check if there was 1 click on the image or 2 clicks on the label
> +      if ((event.detail == 1 && event.originalTarget.localName == "image") ||
> +        (event.detail == 2 && event.originalTarget.localName == "label"))

indention is off. should be
 ((event.detail
  (event.detail

@@ +45,2 @@
>  
> +      if (event.originalTarget.localName == "button")

guess this should be else if? please also add braces

@@ +59,5 @@
> +  /**
> +   * Takes as input two contact elements (imIContact type) and compares
> +   * - their nicknames alphabetically (case insensitive). This method
> +   * - behaves as a callback that Array.prototype.sort accepts as a
> +   * - parameter.

please remove the slashes from the start of the lines

@@ +63,5 @@
> +   * - parameter.
> +   */
> +  sortComparator(contactA, contactB) {
> +    if (contactA.statusType != contactB.statusType)
> +      return contactB.statusType - contactA.statusType;

please add braces for readability

@@ +71,5 @@
> +  }
> +
> +  addContact(contact, tagName) {
> +    if (this.contactsById.hasOwnProperty(contact.id))
> +      return null;

here too

@@ +75,5 @@
> +      return null;
> +
> +    let contactElt;
> +    if (tagName) {
> +      contactElt = document.createElement(tagName);

just wondering, which case is this?

@@ +79,5 @@
> +      contactElt = document.createElement(tagName);
> +    } else {
> +      contactElt = document.createElement("richlistitem", { is: "chat-contact" });
> +    }
> +    if (this.hasAttribute("closed"))

Can we update the closed attribute and instead use a class? (I dislike using made-up attributes for things that are really about making css selectors find something).

@@ +130,5 @@
>  
> +  removeContact(contactForID) {
> +    let contact = this.contactsById[contactForID.id];
> +    if (!contact) {
> +      throw new Error("Removing a contact that isn't here?");

please change this to "Can't remove contact for id=" + ontactForID.id);

@@ +152,2 @@
>  
> +  _updateClosedState(closed) {

I suspect his could actually just be done automagically through the css...

@@ +158,2 @@
>  
> +  close() {

this looks like it's actually toggleClosed() so please update the name

::: mail/components/im/jar.mn
@@ +20,4 @@
>      content/messenger/chat/imContextMenu.js              (content/imContextMenu.js)
>      content/messenger/chat/imStatusSelector.js           (content/imStatusSelector.js)
>      content/messenger/chat/chat-contact.js               (content/chat-contact.js)
> +		content/messenger/chat/chat-group.js                 (content/chat-group.js)

indention off
Attachment #9057474 - Flags: review?(mkmelin+mozilla) → feedback+
Reporter

Updated

a month ago
Status: NEW → ASSIGNED
Assignee

Comment 3

a month ago

(In reply to Magnus Melin [:mkmelin] from comment #2)

just wondering, which case is this?

This is for inserting imconv binding which will be converted to chat-conv after this binding.

@@ +79,5 @@

  contactElt = document.createElement(tagName);
} else {
  contactElt = document.createElement("richlistitem", { is: "chat-contact" });
}
if (this.hasAttribute("closed"))

Can we update the closed attribute and instead use a class? (I dislike using
made-up attributes for things that are really about making css selectors
find something).

@@ +152,2 @@

_updateClosedState(closed) {

I suspect his could actually just be done automagically through the css...

I think it will create very complex css as chat-group, chat-contact and chat-conv are sibling nodes. None of them is a parent of others. So we need Javascript to iterate through the list anyway. What do you suggest?

Reporter

Comment 4

a month ago

We can go with the current js approach. Perhaps later...

Assignee

Comment 5

a month ago
Attachment #9057474 - Attachment is obsolete: true
Attachment #9058193 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 6

a month ago
Comment on attachment 9058193 [details] [diff] [review]
Bug-1538550_richlistitem_chat-group.patch

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

Please address the part about using a class for closed instead of a custom attribute
Attachment #9058193 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 7

a month ago
Comment on attachment 9058193 [details] [diff] [review]
Bug-1538550_richlistitem_chat-group.patch

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

::: mail/components/im/content/chat-group.js
@@ +8,4 @@
>  
> +/**
> +  * The MozChatGroup widget displays chat group name and behave as a
> +  * twisty for expansion for chat contacts i.e. "Conversations",

expansion twisty for groups such as
Assignee

Comment 8

a month ago
Attachment #9058193 - Attachment is obsolete: true
Attachment #9058872 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 9

a month ago
Comment on attachment 9058872 [details] [diff] [review]
Bug-1538550_richlistitem_chat-group.patch

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

Looks good. r=mkmelin with the change below

::: mail/components/im/content/chat-group.js
@@ +176,5 @@
> +      this.classList.remove("closed");
> +      this.setAttribute("aria-expanded", "true");
> +      this._updateClosedState(false);
> +    } else {
> +      this.classList.add("closed", "true");

.add("closed")

Otherwise you're adding a class "true"
Attachment #9058872 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 11

a month ago
Attachment #9058872 - Attachment is obsolete: true
Attachment #9059038 - Flags: review+
Assignee

Updated

a month ago
Keywords: checkin-needed

Comment 12

a month ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/42139c6ab122
[de-xbl] convert group binding to <richlistitem is='chat-group'>. r=mkmelin

Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

a month ago
Target Milestone: --- → Thunderbird 68.0
Regressions: 1545398
You need to log in before you can comment on or make changes to this bug.