Closed
Bug 1534511
Opened 6 years ago
Closed 6 years ago
[de-xbl] Migrate mail-multi-emailheaderfield to custom element.
Categories
(Thunderbird :: Mail Window Front End, task)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 68.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #9050216 -
Flags: review?(mkmelin+mozilla)
Comment 3•6 years ago
|
||
Comment on attachment 9050216 [details] [diff] [review] mail-multi-emailheaderfield.patch Review of attachment 9050216 [details] [diff] [review]: ----------------------------------------------------------------- Doesn't work. If there are more than one email I get JavaScript error: chrome://messenger/content/mailWidgets.js, line 896: TypeError: this.emailAddresses is undefined ::: mail/base/content/mailWidgets.js @@ +648,5 @@ > + * > + * extends {MozXULElement} > + */ > +class MozMailMultiEmailHeaderField extends MozXULElement { > + connectedCallback() { add a delayConnectedCallback() check @@ +664,5 @@ > + // The number of lines of addresses we will display before adding a (more) indicator to the > + // widget. This can be increased using the preference mailnews.headers.show_n_lines_before_more. > + this.maxLinesBeforeMore = 1; > + > + remove the double extra empty line @@ +712,5 @@ > + } > + > + /** > + * Method used to add an address to this widget. > + * addresses is an object with 3 properties: displayName, emailAddress and fullAddress. please see this on how to document parameters which are objects: http://usejsdoc.org/tags-param.html the comment is also wrong: there is no "addresses"
Attachment #9050216 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9050216 -
Attachment is obsolete: true
Attachment #9051493 -
Flags: review?(mkmelin+mozilla)
Comment 5•6 years ago
|
||
Comment on attachment 9051493 [details] [diff] [review] mail-multi-emailheaderfield.patch Review of attachment 9051493 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +643,5 @@ > customElements.define("menulist-editable", MozMenulistEditable, { extends: "menulist" }); > }); > + > +/** > + * multi-emailHeaderField presents multiple emailheaderfields with a toggle. The mail-multiemail-headerfield widgets shows multiple emails. It collapses long rows and allows toggling the full view open. This widget is typically used in the message header pane to show addresses for To, Cc, Bcc, and any other addressing type header that can contain more than one mailbox. @@ +653,5 @@ > + if (this.delayConnectedCallback()) { > + return; > + } > + > + this.textContent = ""; while this clears out the children, I think checking hasChildNodes() is better, since then you don't need to do the unneeded other work either @@ +655,5 @@ > + } > + > + this.textContent = ""; > + > + this.addChildren(); just used once, so just inline that function @@ +716,5 @@ > + /** > + * Method used to add an address to this widget. > + * > + * @param {Object} address address is an object with 3 properties: displayName, emailAddress > + * and fullAddress Please see my previous review comments regarding this. You didn't update this. @@ +732,5 @@ > + > + /** > + * Private method used to set properties on an address node. > + */ > + updateEmailAddressNode(emailNode, address) { if it's private. just move it to _updateEmailAddressNode @@ +752,5 @@ > + /** > + * Private method used to create email address nodes for either our short or long view. > + * > + * @param {Boolean} all If false, show only a few addresses + "more". > + * @return {Integer} Number of addresses we have put into the list. I don't think there needs to be much if any space between here @@ +930,5 @@ > + } > + let ttText = tttArray.join(", "); > + > + let remainingAddresses = addresses.length - tttArray.length; > + // Mot all missing addresses fit in the tooltip. Not @@ +1012,5 @@ > + this.clearChildNodes(); > + } > +} > + > +customElements.define("mail-multi-emailHeaderField", MozMailMultiEmailHeaderField); let's not do any camelCasing for CE maybe mail-multiemailheaderfield and MozMailMultiemailHeaderfield
Attachment #9051493 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #9051493 -
Attachment is obsolete: true
Attachment #9053930 -
Flags: review?(mkmelin+mozilla)
Comment 8•6 years ago
|
||
Comment on attachment 9053930 [details] [diff] [review] mail-multi-emailheaderfield.patch Review of attachment 9053930 [details] [diff] [review]: ----------------------------------------------------------------- Except for the comments, this looks good ::: mail/base/content/mailWidgets.js @@ +752,5 @@ > +class MozMailMultiEmailheaderfield extends MozXULElement { > + constructor() { > + super(); > + > + this.mAddresses = ""; hmm @@ +772,5 @@ > + // This field is used to specify the maximum number of addresses in the more button tooltip > + // text. > + this.tooltipLength = 20; > + > + this.mAddresses = []; ugh! Let's drop the m part here and use this.addresses instead @@ +780,5 @@ > + if (this.delayConnectedCallback()) { > + return; > + } > + > + if (!this.hasChildNodes()) { prefer an early return @@ +814,5 @@ > + return this.tooltipLength; > + } > + > + /** > + * Method used to add an address to this widget. Add an address to be shown in this widget. @@ +848,5 @@ > + UpdateEmailNodeDetails(address.emailAddress, emailNode); > + } > + } catch (ex) { > + dump(`UpdateEmailNodeDetails failed: ${ex}\n`); > + } we can remove this try/catch @@ +858,5 @@ > + * @param {Boolean} all If false, show only a few addresses + "more". > + * @return {Integer} Number of addresses we have put into the list. > + */ > + _fillAddressesNode(all) { > + const addressesNode = this.emailAddresses; I don't think we need this local temp var @@ +928,5 @@ > + // Hide the last node spanning into the additional line (n>1) > + // also hide it if <30px left after sliding the address (n=1) > + // or if the last address would be truncated without "more" > + if (curLine >= this.maxLinesBeforeMore && > + (this.maxLinesBeforeMore > 1 || we usually indent these as if (curLine >= this.maxLinesBeforeMore && (this.maxLinesBeforeMore > 1 ||
Attachment #9053930 -
Flags: review?(mkmelin+mozilla) → feedback+
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #9053930 -
Attachment is obsolete: true
Attachment #9054219 -
Flags: review?(mkmelin+mozilla)
Comment 10•6 years ago
|
||
Comment on attachment 9054219 [details] [diff] [review] mail-multi-emailheaderfield.patch Review of attachment 9054219 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin ::: mail/base/content/mailWidgets.js @@ +1074,5 @@ > + // Re-render the node, this time with all the addresses. > + this._fillAddressesNode(true); > + // Compute height of 'expandedHeaderView' from 'expandedHeadersBox'. > + let expandedHeaderView = document.getElementById("expandedHeaderView"); > + expandedHeaderView.setAttribute( you don't need the variable for the view. just do document.getElementById("expandedHeaderView").setAttribute
Attachment #9054219 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #9054219 -
Attachment is obsolete: true
Attachment #9054440 -
Flags: review+
Assignee | ||
Comment 12•6 years ago
|
||
I have made some changes to test files, could you please approve them quickly?
Attachment #9054440 -
Attachment is obsolete: true
Attachment #9054682 -
Flags: review+
Attachment #9054682 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 14•6 years ago
|
||
Comment on attachment 9054682 [details] [diff] [review] mail-multi-emailheaderfield.patch Review of attachment 9054682 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #9054682 -
Flags: review+
Attachment #9054682 -
Flags: feedback?(mkmelin+mozilla)
Updated•6 years ago
|
Keywords: checkin-needed
Comment 15•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5ffce18591e3
Migrate mail-multi-emailheaderfield binding to custom element. r=mkmelin
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•