Closed Bug 1519800 Opened 6 years ago Closed 6 years ago

[de-xbl] item-date-row

Categories

(Calendar :: General, enhancement)

Product:
Calendar
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(1 file, 6 obsolete files)

Bug-1519800_de-xbl_item-date-row.patch
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
Attached patch bug1519800_De-XBL-item-date-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9036946 - Flags: review?(mkmelin+mozilla)
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?

(In reply to Arshad Khan [:arshad] from comment #2)

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).

textbox is a XBL binding so I thought I need to add it in parseXULToFragment.

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.

Should I just add add a condition over there that do only if is not equal to null ?

why -moz-grid-line?

I have replicated row behavior here. As I need two elements under row element to make column behaviour working. If I add item-date-row element under the row element, it will take as a single element and will display it as a single column. So I removed the row element and gave display styling to item-date-row.

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 ...>
Attachment #9036946 - Flags: review?(mkmelin+mozilla) → review-

Please also add more context to your patches. 8 or 9. In .hgrc, add this

[defaults]
diff = -p -U 9

Status: NEW → ASSIGNED
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..

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)

^

(In reply to Magnus Melin [:mkmelin] from comment #5)

Please also add more context to your patches. 8 or 9. In .hgrc, add this
[defaults]
diff = -p -U 9

Must be 8 so we can compare patches.

(In reply to Arshad Khan [:arshad] from comment #6)

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

All the more reason not to have getter then :)

Please ignore ^^^. Doh.

Attached patch bug1519800_De-XBL-item-date-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9036946 - Attachment is obsolete: true
Attachment #9037407 - Flags: review?(mkmelin+mozilla)
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")
Attachment #9037407 - Flags: review?(mkmelin+mozilla) → review-
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

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

We are applying align property to label only. We are changing label's orientation on different locations. So I think row is not getting affected anywhere.

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

Without flex, it is creating a problem in setting up the width. It shows fixed width but we want 100% width so I have added flex. I will shift it to markup.

Attached patch bug1519800_De-XBL-item-date-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9037407 - Attachment is obsolete: true
Attachment #9037936 - Flags: review?(philipp)
Attachment #9037936 - Flags: feedback?(mkmelin+mozilla)
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)
Attachment #9037936 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attached patch bug1519800_De-XBL-item-date-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9037936 - Attachment is obsolete: true
Attachment #9037936 - Flags: review?(philipp)
Attachment #9037946 - Flags: review?(philipp)
Attachment #9037946 - Flags: feedback+
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
Attachment #9037946 - Flags: review?(philipp) → review+

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #19)

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?

IIRC, the conclusion was that in theory that should work. Whether the overlay loader handles this or not, I don't know (it handles loading js manually and slightly differently than the standard way).

(In reply to Magnus Melin [:mkmelin] from comment #20)

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #19)

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?

IIRC, the conclusion was that in theory that should work. Whether the overlay loader handles this or not, I don't know (it handles loading js manually and slightly differently than the standard way).

Ah ok, I'll try to remember. The overlay loader does not support this, so we're waiting on bug 1311728. Carry on without a module then.

Attached patch De-XBL_item-date-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9037946 - Attachment is obsolete: true
Attachment #9043126 - Flags: review?(philipp)
Attachment #9043126 - Flags: feedback+
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?
Attachment #9043126 - Flags: review?(philipp) → review+
Attached patch De_XBL_item-date-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9043126 - Attachment is obsolete: true
Attachment #9047535 - Flags: review+
Summary: De-XBL item-date-row → [de-xbl] item-date-row
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/17c470797127
[de-xbl] convert item-date-row binding to custom element. r=philipp

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 7.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: