Closed Bug 1491756 Opened Last year Closed Last year

[de-xbl] Convert mail-tagfield to custom element.

Categories

(Thunderbird :: Mail Window Front End, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: arshad, Assigned: arshad)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 12 obsolete files)

No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9009543 - Flags: feedback?(mkmelin+mozilla)
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)
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9009543 - Attachment is obsolete: true
Attachment #9011480 - Flags: review?(mkmelin+mozilla)
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)
 
> please inline

isn't it better to create variables to hold nodes if they are used more than once?
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9011480 - Attachment is obsolete: true
Attachment #9012132 - Flags: feedback?(mkmelin+mozilla)
(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 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+
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9012132 - Attachment is obsolete: true
Attachment #9012534 - Flags: review+
Magnus, is this what you wanted? Rename "aTags" to "a"
Flags: needinfo?(mkmelin+mozilla)
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9012534 - Attachment is obsolete: true
Attachment #9012535 - Flags: review+
Flags: needinfo?(mkmelin+mozilla)
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9012535 - Attachment is obsolete: true
Attachment #9012536 - Flags: review+
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+
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Sorry, i got confused bw feedback and review..
Attachment #9012536 - Attachment is obsolete: true
Attachment #9012863 - Flags: review?(mkmelin+mozilla)
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-
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9012863 - Attachment is obsolete: true
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9013315 - Attachment is obsolete: true
Attachment #9013319 - Flags: review?(mkmelin+mozilla)
(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 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-
Blocks: 1496175
No longer blocks: 1496175
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9013319 - Attachment is obsolete: true
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9014122 - Attachment is obsolete: true
Attached patch mail-tagfield.patch (obsolete) — Splinter Review
Attachment #9014125 - Attachment is obsolete: true
(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..
Attachment #9014133 - Flags: review?(mkmelin+mozilla)
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.
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..
Attachment #9014133 - Attachment is obsolete: true
Attachment #9014133 - Flags: review?(mkmelin+mozilla)
Attachment #9014394 - Flags: review?(mkmelin+mozilla)
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+
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/205e5b839218
Convert mail-tagfield to custom element; r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.