Arshad Khan [:arshad] Assignee
	Description • 6 years ago

      No description provided.

	Magnus Melin [:mkmelin]
	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"

	Magnus Melin [:mkmelin]
	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

	Arshad Khan [:arshad] 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?

	Magnus Melin [:mkmelin]
	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.

	Magnus Melin [:mkmelin]
	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)

	Arshad Khan [:arshad] Assignee
	Comment 10 • 6 years ago

Magnus, is this what you wanted? Rename "aTags" to "a"

	Magnus Melin [:mkmelin]
	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....

	Arshad Khan [:arshad] Assignee
	Comment 14 • 6 years ago

Sorry, i got confused bw feedback and review..

	Magnus Melin [:mkmelin]
	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......

	Magnus Melin [:mkmelin]
	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.)

	Magnus Melin [:mkmelin]
	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

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] Assignee
	Comment 24 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6b2da4aaa8be19ea8f866b1401ced24c8322a353

	Magnus Melin [:mkmelin]
	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.

	Arshad Khan [:arshad] 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..

	Magnus Melin [:mkmelin]
	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

	Pulsebot
	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