[de-xbl] Migrate result-message to custom element.

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 66.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

4 months ago
No description provided.
Assignee

Updated

4 months ago
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Assignee

Updated

4 months ago
Assignee

Updated

4 months ago
Blocks: 1517523
Assignee

Comment 1

4 months ago
Posted patch result-message.patch (obsolete) — Splinter Review
Attachment #9037587 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9037587 [details] [diff] [review]
result-message.patch

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

::: mail/base/content/glodaFacet.js
@@ +6,5 @@
> +/* global logException, Gloda */
> +
> +ChromeUtils.import("resource:///modules/MailServices.jsm");
> +
> +const createElement = (nodeName, attributes) => {

I don't think this is a particulary good idea, especially as document.createElement also has a similar but incompatible signature.

In general I don't like helper methods that are not useful enough. They add too much mental overhead to be worth it. Granted, it makes things slightly shorter here.

@@ +970,5 @@
>      }
>    }
>  }
>  
> +class MozResultMessage extends HTMLElement {

I've been thinking we should really add some documentation to these as we go. Something like

/**
 * MozResultMessage displays an excerpt of a message. Typically these are used in the gloda
 * results listing, showing the messages that matched.
 */

@@ +1279,5 @@
>  customElements.define("facet-date", MozFacetDate);
>  customElements.define("facet-boolean", MozFacetBoolean);
>  customElements.define("facet-boolean-filtered", MozFacetBooleanFiltered);
>  customElements.define("facet-discrete", MozFacetDiscrete);
> +customElements.define("result-message", MozResultMessage);

please rename to facet-result-message and MozFacetResultMessage
Attachment #9037587 - Flags: review?(mkmelin+mozilla) → feedback+
Assignee

Comment 3

4 months ago
Posted patch result-message.patch (obsolete) — Splinter Review
Attachment #9037587 - Attachment is obsolete: true
Attachment #9037976 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9037976 [details] [diff] [review]
result-message.patch

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

LGTM, r=mkmelin

::: mail/base/content/glodaFacet.js
@@ +1298,5 @@
> +      this.setAttribute("unread", "true");
> +  }
> +}
> +
> +

nit: double empty rows here

@@ +1303,5 @@
>  customElements.define("facet-date", MozFacetDate);
>  customElements.define("facet-boolean", MozFacetBoolean);
>  customElements.define("facet-boolean-filtered", MozFacetBooleanFiltered);
>  customElements.define("facet-discrete", MozFacetDiscrete);
> +customElements.define("facet-result-message", MozFacetResultMessage);

I think we should actually start putting the define straight after the class definition block (where the double empty row is now in this patch)
Attachment #9037976 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 6

4 months ago
Attachment #9037976 - Attachment is obsolete: true
Assignee

Updated

4 months ago
Keywords: checkin-needed

Comment 7

4 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0c2445d75375
Migrate result-message binding to custom element; r=mkmelin

Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 8

4 months ago

Could you please include the word "binding" in the commit message when your migrate a binding. If I don't fix it, it gets landed like this: "Migrate popup-menu to custom element; r=mkmelin". Surely in that case no popup menus got migrated, but instead a binding.

Target Milestone: --- → Thunderbird 66.0
You need to log in before you can comment on or make changes to this bug.