Arshad Khan [:arshad]
	Comment 2 • 6 years ago

Comment on attachment 9036946 [details] [diff] [review]
bug1519800_De-XBL-item-date-row.patch

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

::: calendar/base/content/calendar-item-bindings.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +  * License, v. 2.0. If a copy of the MPL was not distributed with this
> +  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* global MozXULElement */

maybe add `cal` into this list of globals

@@ +11,5 @@
> +            return ["align"];
> +        }
> +
> +        connectedCallback() {
> +            this.appendChild(MozXULElement.parseXULToFragment(`

Use parseXULToFragment only when you want to use DTD values(1) or the amount of children that you are appending is way too much. Example for case 1 - https://searchfox.org/comm-central/source/mailnews/base/search/content/searchWidgets.js#43 and example for case 2 - https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1512941&attachment=9033534 (see moztimpickergrids class).

@@ +24,5 @@
> +            this._updateAttributes();
> +        }
> +
> +        _updateAttributes() {
> +            let headerLabel = this.querySelector("#item-datetime-label");

always make sure the element is available and not null, when you are setting an attribute or anything else on the node. here if I create MozItemDateRow by js and set align ATtribute on the element then an error will be thrown, becuase I created the element but never appended it to DOM and thus the connectedCallback was never called and #item-datetime-label was never added as a child.

::: calendar/base/content/dialogs/calendar-summary-dialog.xul
@@ +50,4 @@
>            src="chrome://calendar/content/calendar-item-editing.js"/>
>    <script type="application/javascript"
>            src="chrome://calendar/content/calApplicationUtils.js"/>
> +  <script type="application/javascript" 

empty space.

::: calendar/base/themes/common/calendar-task-view.css
@@ +40,5 @@
>      border: none;
>  }
>  
> +.item-date-row {
> +    display: -moz-grid-line !important;

why -moz-grid-line?

	Magnus Melin [:mkmelin] Reporter
	Comment 4 • 6 years ago

Comment on attachment 9036946 [details] [diff] [review]
bug1519800_De-XBL-item-date-row.patch

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

Good start, but seem the comments. The end result may be a bit more easy.
(And some comments may be obsolete, once you make the element be a child of row)

::: calendar/base/content/calendar-item-bindings.js
@@ +7,5 @@
> +"use strict";
> +{
> +    class MozItemDateRow extends MozXULElement {
> +        static get observedAttributes() {
> +            return ["align"];

shouldn't you also observe mode, taskStartLabel, eventEndLabel

@@ +11,5 @@
> +            return ["align"];
> +        }
> +
> +        connectedCallback() {
> +            this.appendChild(MozXULElement.parseXULToFragment(`

probably need parseXULToFragment here due to the textbox binding yes

@@ +13,5 @@
> +
> +        connectedCallback() {
> +            this.appendChild(MozXULElement.parseXULToFragment(`
> +                <label id="item-datetime-label" class="headline"></label>
> +                <textbox readonly="true" class="selectable-label plain" id="item-datetime-value"></textbox>

ugh. should't really put id'd elements into a custom element. they are meant to be reusable.
usually, just this.querySelector on class instead, or if you're constucting the content manually, hold a reference to it, like this._textbox = ....

@@ +24,5 @@
> +            this._updateAttributes();
> +        }
> +
> +        _updateAttributes() {
> +            let headerLabel = this.querySelector("#item-datetime-label");

yep, in _updateAttributes() you ned to check if the connectedCallback ran. I.e. if (!this.firstChild) return;

@@ +37,5 @@
> +            if (this.hasAttribute("mode")) {
> +                return this.getAttribute("mode");
> +            } else {
> +                return "start";
> +            }

while you're here, please fix the else after return. So

if (this.hasAttribute("mode")) {
  return this.getAttribute("mode");
}
return "start";

@@ +95,5 @@
> +            }
> +        }
> +
> +        get item() {
> +            return this.mItem;

please make this.mItem just this.item (change mItem to item). then you don't need the getter here (but perhaps eslint will insist on it still since you have a setter)

::: calendar/base/content/calendar-task-view.xul
@@ +210,5 @@
> +                <item-date-row class="item-date-row"
> +                               id="task-start-row"
> +                               mode="start"
> +                               taskStartLabel="&calendar.task-details.start.label;"
> +                               align="end"/>

I think it would actually make more sense to not have the custom element be a row, but instead you'd have something like a calendar-item-date (which you'd create)

<row>
  <label ....>
  <calendar-item-date ...>

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

Comment on attachment 9036946 [details] [diff] [review]
bug1519800_De-XBL-item-date-row.patch

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

::: calendar/base/content/calendar-item-bindings.js
@@ +41,5 @@
> +            }
> +        }
> +
> +        set item(val) {
> +            this.mItem = val;

if you change mItem to item then this line will turn into an infinite loop as the setter function will call itself again..

	Magnus Melin [:mkmelin] Reporter
	Comment 12 • 6 years ago

Comment on attachment 9037407 [details] [diff] [review]
bug1519800_De-XBL-item-date-row.patch

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

This is more verbose then earlier, but I still think it's better this way. Less magic

::: calendar/base/content/calendar-item-bindings.js
@@ +13,5 @@
> +
> +        connectedCallback() {
> +            this.appendChild(MozXULElement.parseXULToFragment(`
> +                <textbox readonly="true" class="selectable-label plain" anonid="item-datetime-value"></textbox>
> +            `));

Usually we remove the anonid (unless there's reason to keep), since that's not webby but just an xbl thing.

@@ +19,5 @@
> +            let taskTextbox = this.querySelector(".selectable-label");
> +            taskTextbox.setAttribute("flex", "1");
> +        }
> +
> +        attributeChangedCallback(name, oldValue, newValue) { }

no point in having this, or the observedAttributes() if you're not going to do anything with it. But, you should react to changes in mode I think.

@@ +62,5 @@
> +            return this.mItem;
> +        }
> +    }
> +
> +    customElements.define("item-date-row", MozItemDateRow);

please rename this, it's not a row. I suggest "calendar-item-date" (and the class accordingly MozCalendarItemDate)

::: calendar/base/content/calendar-task-view.js
@@ +113,5 @@
> +            if (tastStartDate) {
> +                taskStartRowLabel.style.visibility = "visible";
> +            } else {
> +                taskStartRowLabel.style.visibility = "collapse";
> +            }

for these ternary would be more compact, as

 taskStartRowLabel.style.visibility = (taskStartDate) ? visible" : "collapse";


Also, typo. task, not tast

::: calendar/base/content/calendar-task-view.xul
@@ +210,5 @@
> +                <row class="item-date-row">
> +                  <label id="task-start-row-label"
> +                         class="headline"
> +                         value="&calendar.task-details.start.label;"
> +                         align="end"/>

align should probably go on the <row>, only, no?

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +84,5 @@
> +    let itemStartRowLabel = document.getElementById("item-start-row-label");
> +    let itemStartDate = item[cal.dtz.startDateProp(item)];
> +    if (itemStartDate) {
> +        itemStartRowLabel.style.visibility = "visible";
> +        itemStartRowLabel.setAttribute("value", cal.item.isToDo(item) ? itemStartRowLabel.getAttribute("taskStartLabel") : itemStartRowLabel.getAttribute("eventStartLabel"));

it's shorter to do 

itemStartRowLabel.getAttribute(cal.item.isToDo(item) ? "taskStartLabel" : "eventStartLabel")

	Magnus Melin [:mkmelin] Reporter
	Comment 13 • 6 years ago

Comment on attachment 9037407 [details] [diff] [review]
bug1519800_De-XBL-item-date-row.patch

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

::: calendar/base/content/calendar-item-bindings.js
@@ +16,5 @@
> +                <textbox readonly="true" class="selectable-label plain" anonid="item-datetime-value"></textbox>
> +            `));
> +            this.mItem = null;
> +            let taskTextbox = this.querySelector(".selectable-label");
> +            taskTextbox.setAttribute("flex", "1");

Oh, I forgot, setting flex here makes little sense. You can just put it in the markup if it's needed

	Magnus Melin [:mkmelin] Reporter
	Comment 17 • 6 years ago

Comment on attachment 9037936 [details] [diff] [review]
bug1519800_De-XBL-item-date-row.patch

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

Seems to work fine. f+

To preserve (some history) you could do

hg mv --after calendar/base/content/calendar-item-bindings.xml calendar/base/content/calendar-item-bindings.js

::: calendar/base/content/calendar-task-view.xul
@@ +220,5 @@
> +                         class="headline"
> +                         value="&calendar.task-details.due.label;"
> +                         align="end"/>
> +                  <calendar-item-date id="task-due-row"
> +                                 mode="end"/>

nit: odd alignment here. put mode on the same line perhaps since it fits. (or align id and mode under each other)

::: calendar/base/content/dialogs/calendar-summary-dialog.xul
@@ +163,5 @@
> +            <label id="item-due-row-label"
> +                   taskDueLabel="&read.only.task.due.label;"
> +                   eventEndLabel="&read.only.event.end.label;"
> +                   align="center"/>
> +            <calendar-item-date id="item-end-row"

it is slightly confusing that you're re-using the id. could be better to let the ids stay, and then find another one like "item-date-row-end-date" (applies to the similar cases too in this patch)

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 19 • 6 years ago

Comment on attachment 9037946 [details] [diff] [review]
bug1519800_De-XBL-item-date-row.patch

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

Just a few minor nits and code style comments. r=philipp with these changes:

::: calendar/base/content/calendar-item-bindings.js
@@ +5,5 @@
> +/* global MozXULElement, cal */
> +
> +"use strict";
> +{
> +    class MozCalendarItemDate extends MozXULElement {

I've again forgotten what you and Magnus said about this, but can we use <script type="module"> to avoid the need for the extra scope block?

Also, maybe you can add docs on this class.

@@ +29,5 @@
> +        _updateAttributes() {
> +            this.setAttribute("mode", this.getAttribute("mode") || "");
> +        }
> +
> +        get mode() {

It would also be great to add jsdoc headers for the remaining methods. I'm ok with skipping web components methods like connectedCallback and observedAttributes, but those custom to this element should be documented.

::: calendar/base/content/calendar-task-view.xul
@@ +212,5 @@
> +                         class="headline"
> +                         value="&calendar.task-details.start.label;"
> +                         align="end"/>
> +                  <calendar-item-date id="task-start-date"
> +                                      mode="start"/>

This can go in one line

@@ +220,5 @@
> +                         class="headline"
> +                         value="&calendar.task-details.due.label;"
> +                         align="end"/>
> +                  <calendar-item-date id="task-due-date"
> +                                      mode="end"/>

Same

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +84,5 @@
> +    let itemStartRowLabel = document.getElementById("item-start-row-label");
> +    let itemStartDate = item[cal.dtz.startDateProp(item)];
> +    itemStartRowLabel.style.visibility = itemStartDate ? "visible" : "collapse";
> +    if (itemStartDate) {
> +        itemStartRowLabel.setAttribute("value", itemStartRowLabel.getAttribute(cal.item.isToDo(item) ? "taskStartLabel" : "eventStartLabel"));

This line looks fairly long. Can you limit it to 100 chars? You can use temporary variables to help.

::: calendar/base/content/dialogs/calendar-summary-dialog.xul
@@ +156,5 @@
> +                   taskStartLabel="&read.only.task.start.label;"
> +                   eventStartLabel="&read.only.event.start.label;"
> +                   align="center"/>
> +            <calendar-item-date id="item-date-row-start-date"
> +                                mode="start"/>

One line

@@ +164,5 @@
> +                   taskDueLabel="&read.only.task.due.label;"
> +                   eventEndLabel="&read.only.event.end.label;"
> +                   align="center"/>
> +            <calendar-item-date id="item-date-row-end-date"
> +                                mode="end"/>

One line

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 23 • 6 years ago

Comment on attachment 9043126 [details] [diff] [review]
De-XBL_item-date-row.patch

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

::: calendar/base/content/calendar-item-bindings.js
@@ +39,5 @@
> +
> +    /**
> +     * Called when getting mode of the date. Default is start.
> +     *
> +     * @returns {string} mode of the date(start/end).

Can you leave at least one tab stop between the type and the description? Here and in other comments.

I recently left some advice for how to approach comments on a review for Arshad, maybe you can ask him which one that was and get some additional thoughts.

When writing comments, don't focus on where they are called, but what they do. This getter would just "Returns the mode of the date, defaulting to "start"." Also keep in mind that people reading these comments may not know the rest of the code. Without looking further, I don't know what the valid values for a mode are, nor what kind of mode this is. So a short description of that would be beneficial.

@@ +49,5 @@
> +        return "start";
> +    }
> +
> +    /**
> +     * Called when setting event/task item for custom element.

Same comment on call vs what it does here and below.

::: calendar/base/content/calendar-task-view.js
@@ +108,5 @@
> +            document.getElementById("task-due-date").item = item;
> +
> +            let taskStartRowLabel = document.getElementById("task-start-row-label");
> +            let taskStartDate = item[cal.dtz.startDateProp(item)];
> +            taskStartRowLabel.style.visibility = (taskStartDate) ? "visible" : "collapse";

I don't think you need the extra parens here. Doesn't eslint complain about this?