Closed Bug 1512805 Opened 6 years ago Closed 6 years ago

[de-xbl] Migrate freebusy-row binding to custom element.

Categories

(Calendar :: Lightning Only, enhancement)

Product:
Calendar
Component:
Lightning Only
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 12 obsolete files)

freebusy-row.patch
(deleted), patch
arshad
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
This bug will be applied on top of freebusy-day bug 1511270.
Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9030082 - Flags: feedback?(mkmelin+mozilla)
Attachment #9030082 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attachment #9030082 - Flags: review?(makemyday)
Blocks: tb-war-on-xbl
Depends on: 1509837
Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9030082 - Attachment is obsolete: true
Attachment #9030082 - Flags: review?(makemyday)
Attachment #9050630 - Flags: feedback?(makemyday)
Comment on attachment 9050630 [details] [diff] [review] freebusy-row.patch Review of attachment 9050630 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +302,5 @@ > + * > + * @param {Number} val new mZoomFactor value > + * @returns {Number} new mZoomFactor value > + */ > + set zoomFactor(val) { Please suggest better comment for this one. @@ +395,5 @@ > + > + /** > + * @returns {Number} > + */ > + get numHours() { this one. @@ +403,5 @@ > + > + /** > + * @returns {Number} > + */ > + get contentWidth() { this one. @@ +436,5 @@ > + > + /** > + * @returns {number} Document size > + */ > + get documentSize() { this one as well.

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

  • * @param {Number} val      new mZoomFactor value

  • * @returns {Number}        new mZoomFactor value

  • */
  • set zoomFactor(val) {

Please suggest better comment for this one.

When reading the documentation, what would you expect it to say to be helpful? What can be passed in as val? What kind of number. These are the things the documentation should say.

@@ +395,5 @@

  • /**
  • * @returns {Number}

  • */
  • get numHours() {

this one.

What is the returned value? The name gives you a good hint.

@@ +403,5 @@

  • /**
  • * @returns {Number}

  • */
  • get contentWidth() {

this one.

@@ +436,5 @@

  • /**
  • * @returns {number}        Document size

  • */
  • get documentSize() {

this one as well.

{Number}

Name gives you a good hint here too. For getters like this, "document size" is fairly ok as a comment I think. I'm not looking at the context now, but what is the size? What is the unit, if any.

Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9050630 - Attachment is obsolete: true
Attachment #9050630 - Flags: feedback?(makemyday)
Attachment #9054213 - Flags: review?(makemyday)
Attached patch freebusy-grid.patch (obsolete) (deleted) — — Splinter Review
Attachment #9054213 - Attachment is obsolete: true
Attachment #9054213 - Flags: review?(makemyday)
Attachment #9056271 - Attachment is obsolete: true
Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9056272 - Attachment is obsolete: true
Attachment #9056278 - Flags: review?(philipp)
Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9056278 - Attachment is obsolete: true
Attachment #9056278 - Flags: review?(philipp)
Attachment #9056281 - Flags: review?(philipp)
Comment on attachment 9056281 [details] [diff] [review] freebusy-row.patch Review of attachment 9056281 [details] [diff] [review]: ----------------------------------------------------------------- Please check the documentation again. This is really something you are going to have to put some effort in. You can read many other examples in the source and we've talked about most all of these issues. I know documentation is annoying, but this is just part of writing good code. * Be exact with the types you specify, use the most accurate specialization (e.g. Node vs Element) * Make sure all params and return values are documented * Don't describe properties by saying they return <name of property>. Spend 5 minutes to find out what it does and properly document it. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +436,5 @@ > + return ( > + ( > + this.hoursNode.childNodes[1].boxObject.x - > + this.hoursNode.childNodes[0].boxObject.x > + ) * this.numHours Readability is a bit strange here. Maybe use some local variables? @@ +446,5 @@ > + * > + * @returns {Number} Nearest listbox width > + */ > + get containerWidth() { > + // Step up the hierarchy until we reach the listbox I don't think this comment is necessary, it should be clear what the closest() function does.
Attachment #9056281 - Flags: review?(philipp) → review-
Comment on attachment 9058870 [details] [diff] [review] freebusy-row.patch Review of attachment 9058870 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.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 MozElements, Services, MozXULElement, removeChildren, cal */ Some of these additions should probably use import-globals-from @@ +307,5 @@ > + return this.hoursNodeElem; > + } > + > + /** > + * Gets scroll-container element. Gets the scroll container with the freebusy rows (if that is correct). Don't use technical specifics such as the id of an element. It could change. @@ +352,5 @@ > + } > + > + /** > + * Sets force24Hours property to a new value, updates endHour, startHour to a new value and > + * removes hour node children. This describes each line of code, but you need to stay more high level there. "If true, forces the freebusy view to 24 hours and updates the UI accordingly." @@ +379,5 @@ > + /** > + * Sets start date of the event and make it immutable. > + * > + * @param {calIDateTime} val startDate value > + * @returns {calIDateTime|null} startDate value {?calIDateTime} instead of {calIDateTime|null} here and other places. Also, please align the parameters correctly. @@ +531,5 @@ > + * Sets freebusy-row state according to param entries which is an array of requested freebusy > + * intervals. After the state has been updated we call showState() which will map the entries to > + * attributes on the xul elements. > + * > + * @param {Array} entries List of freebusy entries Array of what? Use {realtype[]} instead. @@ +665,5 @@ > + * Returns new time for the next slot. > + * > + * @param {calDateTime} startTime previous start time > + * @param {calDateTime} endTime previous end time > + * @param {Boolean} allDay Flag telling whether the event is all day or not You are mixing starting with caps and not starting with caps. Please keep this consistent and start with caps.
Attachment #9058870 - Flags: review?(philipp) → review+
Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058870 - Attachment is obsolete: true
Attachment #9058886 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058886 - Attachment is obsolete: true
Attachment #9058900 - Flags: review+
Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058900 - Attachment is obsolete: true
Attachment #9058917 - Flags: review+
Keywords: checkin-needed
Comment on attachment 9058917 [details] [diff] [review] freebusy-row.patch Review of attachment 9058917 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +535,5 @@ > + * > + * @param {?FreeBusyInterval[]} entries List of freebusy entries > + */ > + onFreeBusy(entries) { > + console.log("entries", entries); This is not very useful, is it?

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

This bug will be applied on top of freebusy-day bug 1511270.

Is this still true. That bug isn't ready, so clearing checkin-needed for now.

Keywords: checkin-needed
Comment on attachment 9058917 [details] [diff] [review] freebusy-row.patch Review of attachment 9058917 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +532,5 @@ > + * Sets freebusy-row state according to param entries which is an array of requested freebusy > + * intervals. After the state has been updated we call showState() which will map the entries to > + * attributes on the xul elements. > + * > + * @param {?FreeBusyInterval[]} entries List of freebusy entries ?calIFreeBusyInterval[]

(In reply to Jorg K (GMT+2) from comment #18)

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

This bug will be applied on top of freebusy-day bug 1511270.

Is this still true. That bug isn't ready, so clearing checkin-needed for now.

No it is not.

Attached patch freebusy-row.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058917 - Attachment is obsolete: true
Attachment #9059068 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed

I'm confused now. Ready to go or not?

Flags: needinfo?(arshdkhn1)
Attached patch freebusy-row.patch (deleted) — — Splinter Review
Attachment #9059068 - Attachment is obsolete: true
Flags: needinfo?(arshdkhn1)
Attachment #9059100 - Flags: review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/10c8c0ceb9fb
Migrate freebusy-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: