Closed
Bug 1491756
Opened 6 years ago
Closed 6 years ago
[de-xbl] Convert mail-tagfield to custom element.
Categories
(Thunderbird :: Mail Window Front End, task)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 64.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 12 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9009543 -
Flags: feedback?(mkmelin+mozilla)
Comment 2•6 years ago
|
||
Comment on attachment 9009543 [details] [diff] [review] mail-tagfield.patch Review of attachment 9009543 [details] [diff] [review]: ----------------------------------------------------------------- You didn't include the removals, which makes it hard to read. Also, 2 space indention please. ::: mail/base/content/mailWidgets.js @@ +29,5 @@ > + while (headerValueNode.hasChildNodes()) headerValueNode.lastChild.remove(); > + > + // tokenize the keywords based on ' ' > + var tagsArray = aTags.split(" "); > + for (var index = 0; index < tagsArray.length; index++) { you can move to using let for all the variables, while you're here. eh, and maybe just "i"
Attachment #9009543 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9009543 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9011480 -
Flags: review?(mkmelin+mozilla)
Comment 4•6 years ago
|
||
Comment on attachment 9011480 [details] [diff] [review] mail-tagfield.patch Review of attachment 9011480 [details] [diff] [review]: ----------------------------------------------------------------- No longer applies to tip ::: mail/base/content/mailWidgets.js @@ +5,5 @@ > + > +/* global MozXULElement, MailServices */ > + > +class MozMailHeaderfieldTags extends MozXULElement { > + static get observedAttributes() { unused? @@ +32,5 @@ > + this.updateHeaderValue(); > + } > + > + updateHeaderValue() { > + this.hasAttribute("label") && please use if (....) instead for cases like this But why is it needed? Is it not ok to set label if the "label" wasn't set initially? @@ +46,5 @@ > + // aTags contains a list of actual tag names (not the keys), delimited by spaces > + // each tag name is encoded. > + > + // remove any existing tag items we've appended to the list > + const headerValueNode = this.querySelector(".headerValue"); please inline @@ +47,5 @@ > + // each tag name is encoded. > + > + // remove any existing tag items we've appended to the list > + const headerValueNode = this.querySelector(".headerValue"); > + while (headerValueNode.hasChildNodes()) headerValueNode.lastChild.remove(); and always { } and new line for loops
Attachment #9011480 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 5•6 years ago
|
||
> please inline
isn't it better to create variables to hold nodes if they are used more than once?
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #9011480 -
Attachment is obsolete: true
Attachment #9012132 -
Flags: feedback?(mkmelin+mozilla)
Comment 7•6 years ago
|
||
(In reply to Arshad Khan [:arshad] from comment #5) > > > please inline > > isn't it better to create variables to hold nodes if they are used more than > once? Ah yes this was used more than once. My bad.
Comment 8•6 years ago
|
||
Comment on attachment 9012132 [details] [diff] [review] mail-tagfield.patch Review of attachment 9012132 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me ::: mail/base/content/mailWidgets.js @@ +65,5 @@ > + set headerValue(val) { > + return this.buildTags(val); > + } > + > + buildTags(aTags) { can we make this named tags now that we're moving over (the "a" convention for arguments makes little sense for JavaScript)
Attachment #9012132 -
Flags: feedback?(mkmelin+mozilla) → feedback+
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #9012132 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9012534 -
Flags: review+
Assignee | ||
Comment 10•6 years ago
|
||
Magnus, is this what you wanted? Rename "aTags" to "a"
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #9012534 -
Attachment is obsolete: true
Attachment #9012535 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #9012535 -
Attachment is obsolete: true
Attachment #9012536 -
Flags: review+
Comment 13•6 years ago
|
||
Comment on attachment 9012536 [details] [diff] [review] mail-tagfield.patch Review of attachment 9012536 [details] [diff] [review]: ----------------------------------------------------------------- This wasn't reviewed yet. Review is review and feedback is feedback ::: mail/base/content/mailWidgets.js @@ +66,5 @@ > + return this.buildTags(val); > + } > + > + buildTags(tags) { > + // a contains a list of actual tag names (not the keys), delimited by spaces tags was what I meant yes. but the "a" is wrong of course. Should be // tags....
Attachment #9012536 -
Flags: review+
Assignee | ||
Comment 14•6 years ago
|
||
Sorry, i got confused bw feedback and review..
Attachment #9012536 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9012863 -
Flags: review?(mkmelin+mozilla)
Comment 15•6 years ago
|
||
Comment on attachment 9012863 [details] [diff] [review] mail-tagfield.patch Review of attachment 9012863 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +37,5 @@ > + this.appendChild( > + MozXULElement.parseXULToFragment(` > + <hbox class="headerNameBox" align="start"> > + <label class="headerName" flex="1"></label> > + </hbox> huh? Where did this come from? It's the headerValue you're supposed to be updating/dealing with, right? @@ +52,5 @@ > + } > + > + updateHeaderValue() { > + const headerName = this.querySelector(".headerName"); > + // guarding the below code to get executed from the very first call if (!this.isConnected) { return; } @@ +66,5 @@ > + return this.buildTags(val); > + } > + > + buildTags(tags) { > + // a contains a list of actual tag names (not the keys), delimited by spaces // tags contains......
Attachment #9012863 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 16•6 years ago
|
||
Attachment #9012863 -
Attachment is obsolete: true
Assignee | ||
Comment 17•6 years ago
|
||
Attachment #9013315 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9013319 -
Flags: review?(mkmelin+mozilla)
Comment 18•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #15) > huh? Where did this come from? It's the headerValue you're supposed to be > updating/dealing with, right? You didn't address this. If you don't address review comments you need to comment in the bug on why. (And yes, as one'd imagine this is causing at least a visual glitch with your patch.)
Comment 19•6 years ago
|
||
Comment on attachment 9013319 [details] [diff] [review] mail-tagfield.patch Review of attachment 9013319 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +76,5 @@ > + // screen reader support on the Mac. > + if ("labelElement" in this) { > + let ariaLabel = this.labelElement.value + ": " + tagName; > + label.setAttribute("aria-label", ariaLabel); > + } You'll be able to remove all this accessibility stuff too
Attachment #9013319 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 20•6 years ago
|
||
Attachment #9013319 -
Attachment is obsolete: true
Assignee | ||
Comment 21•6 years ago
|
||
Attachment #9014122 -
Attachment is obsolete: true
Assignee | ||
Comment 22•6 years ago
|
||
Attachment #9014125 -
Attachment is obsolete: true
Assignee | ||
Comment 23•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #18) > (In reply to Magnus Melin [:mkmelin] from comment #15) > > huh? Where did this come from? It's the headerValue you're supposed to be > > updating/dealing with, right? > > You didn't address this. > If you don't address review comments you need to comment in the bug on why. I removed the label entirely and added `headerValue` class to the mail-tagfield like you did in mail-urlfield..
Assignee | ||
Updated•6 years ago
|
Attachment #9014133 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 24•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6b2da4aaa8be19ea8f866b1401ced24c8322a353
Comment 25•6 years ago
|
||
Comment on attachment 9014133 [details] [diff] [review] mail-tagfield.patch Review of attachment 9014133 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +29,5 @@ > } > > +class MozMailHeaderfieldTags extends MozXULElement { > + connectedCallback() { > + ChromeUtils.import("resource:///modules/MailServices.jsm"); This is a bit weird to import here. Is it needed? If so maybe just put it on top of the file instead ::: mail/base/content/msgHdrView.inc @@ +378,4 @@ > <row id="expandedtagsRow" collapsed="true"> > <label id="expandedtagsLabel" class="headerName" > value="&tagsHdr4.label;" control="expandedtagsBox"/> > + <mail-tagfield id="expandedtagsBox" class="headerValue"/> I suppose it's a question of taste, to put it here or automatically add the class for all mail-tagfields.
Assignee | ||
Comment 26•6 years ago
|
||
Comment on attachment 9014133 [details] [diff] [review] mail-tagfield.patch Review of attachment 9014133 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +29,5 @@ > } > > +class MozMailHeaderfieldTags extends MozXULElement { > + connectedCallback() { > + ChromeUtils.import("resource:///modules/MailServices.jsm"); I have seen some fx patches, which also import files in connected callback.. I think it is ok.. I leave the decision on you. @@ +59,5 @@ > + } catch (ex) { > + continue; > + } > + > + let color = MailServices.tags.getColorForKey(tagsArray[i]); if we dont import MailServices.js then, we can't use MailServices here..
Assignee | ||
Comment 27•6 years ago
|
||
Attachment #9014133 -
Attachment is obsolete: true
Attachment #9014133 -
Flags: review?(mkmelin+mozilla)
Attachment #9014394 -
Flags: review?(mkmelin+mozilla)
Comment 28•6 years ago
|
||
Comment on attachment 9014394 [details] [diff] [review] mail-tagfield.patch Review of attachment 9014394 [details] [diff] [review]: ----------------------------------------------------------------- Great, thx! r=mkmelin
Attachment #9014394 -
Flags: review?(mkmelin+mozilla) → review+
Updated•6 years ago
|
Keywords: checkin-needed
Comment 29•6 years ago
|
||
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/205e5b839218 Convert mail-tagfield to custom element; r=mkmelin
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 64.0
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•