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

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Comment 1

[Arshad Khan [:arshad]]

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

Comment 2

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 3

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 4

[Arshad Khan [:arshad]]

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.

Comment 5

[Magnus Melin [:mkmelin]]

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

Comment 6

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 7

[Arshad Khan [:arshad]]

Attachment: freebusy-grid.patch

Comment 8

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 9

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 10

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 12

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

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

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 14

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 15

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 16

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 17

[Jorg K (CEST = GMT+2)]

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?

Comment 18

[Jorg K (CEST = GMT+2)]

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

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

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[]

Comment 20

[Arshad Khan [:arshad]]

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

Comment 21

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 22

[Jorg K (CEST = GMT+2)]

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

Comment 23

[Arshad Khan [:arshad]]

Attachment: freebusy-row.patch

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/10c8c0ceb9fb
Migrate freebusy-row binding to custom element. r=philipp