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

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Comment 1

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9028858 [details] [diff] [review]
freebusy-day.patch

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

Could lose the mNotation for member variables while you're touching all of this

::: calendar/base/content/dialogs/calendar-event-dialog.css
@@ -36,5 @@
>      -moz-user-focus: normal;
>  }
>  
>  freebusy-day {
> -    -moz-binding: url(chrome://calendar/content/calendar-event-dialog-freebusy.xml#freebusy-day);

you didn't include the freebusy-day removal in the patch, so it's not easy to compare

Comment 3

[Magnus Melin [:mkmelin]]

I get a bunch of 
JavaScript error: chrome://calendar/content/calendar-event-dialog-freebusy.xml, line 1302: TypeError: this.getListItem(...) is undefined; can't access its "getElementsByTagName" property

... that would be in the "getFreeBusyElement" method

Comment 4

[Arshad Khan [:arshad]]

(In reply to undefined from comment #undefined)
> 

they are not caused by this patch. if you see the logs without applying this patch, you ll even then find them there.

Comment 5

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #3)
> I get a bunch of 
> JavaScript error:
> chrome://calendar/content/calendar-event-dialog-freebusy.xml, line 1302:
> TypeError: this.getListItem(...) is undefined; can't access its
> "getElementsByTagName" property
> 
> ... that would be in the "getFreeBusyElement" method

they are not caused by this patch. if you see the logs without applying this patch, you ll even then find them there.

Comment 6

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 7

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 8

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

This patch will be applied on top of Bug 1509837.

Comment 9

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

This patch applies on top of Bug 1509837.

Comment 10

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

This patch applies on top of Bug 1509837.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9030074 [details] [diff] [review]
freebusy-day.patch

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

LGTM

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-bindings.js
@@ +177,5 @@
> +        }
> +
> +        // First set the formatted date string as title
> +        let dateValue = this.zoomFactor > 100 ? this.dateFormatter.formatDateShort(date)
> +            : this.dateFormatter.formatDateLong(date);

: goes on the previous line

Comment 12

[Arshad Khan [:arshad]]

(In reply to undefined from comment #undefined)
> 

https://searchfox.org/comm-central/source/calendar/.eslintrc.js#372

Comment 13

[Arshad Khan [:arshad]]

according to this rule it should be at after the line break..

Comment 14

[Magnus Melin [:mkmelin]]

Ok, looks like a calendar only thing. It's an non-default configuration not used elsewhere in mozilla eslint rules either.

Comment 15

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Removed the dependence on scroll-container element.

Comment 16

[[:MakeMyDay]]

Comment on attachment 9050608 [details] [diff] [review]
freebusy-day.patch

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

I haven't got to tested the patch, so some comments mainly on the doc blocks and some style nits for now. Pleaese check especially the doc blocks, since the comments apply on all of them.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +264,5 @@
>  
>  customElements.define("calendar-event-freebusy-timebar", MozCalendarEventFreebusyTimebar);
> +
> +
> +class MozFreebusyDay extends MozXULElement {

Please add a doc block for the class itself describing its purpose.

@@ +303,5 @@
> +    /**
> +     * Zooms time slots and removes children.
> +     *
> +     * @param {Number} val      new mZoomFactor value
> +     * @returns {Number}        new mZoomFactor value

The documentation should not expose details of the implementation like internal variable names but give a comprehensible description of the purpose of the function. So, |new zoom factor| seems suitable here for the parameter and the return value description, like |Sets the zoom factor for the free busy grid| does for the setter description.

This comment applies in general for all doc blocks in this file, so please revisit them.

@@ +348,5 @@
> +    /**
> +     * Sets start date of the event and make it immutable.
> +     *
> +     * @param {calDateTime} val     mStartDate value
> +     * @returns {calDateTime}       mStartDate value

calIDateTime

@@ +409,5 @@
> +    /**
> +     * Updates the date object passed according to startHour and endHour.
> +     *
> +     * @param {calDateTime} val        date object to be modified
> +     * @returns {calDateTime}       modified date object

calIDateTime

@@ +423,5 @@
> +        date.isDate = false;
> +
> +        if (!this.dateFormatter) {
> +            this.dateFormatter = Cc["@mozilla.org/calendar/datetime-formatter;1"]
> +                .getService(Ci.calIDateTimeFormatter);

Please add some indentation here, so that the leading dot is vertically aligned with the [ above

@@ +428,5 @@
> +        }
> +
> +        // First set the formatted date string as title
> +        let dateValue = this.zoomFactor > 100 ? this.dateFormatter.formatDateShort(date)
> +            : this.dateFormatter.formatDateLong(date);

Please add some indentation here, so that the : is vertically aligned with the ? above

@@ +439,5 @@
> +        if (hours.childNodes.length <= 0) {
> +            let template = document.createXULElement("text");
> +            template.className = "freebusy-timebar-hour";
> +            let count = Math.ceil(
> +                (this.endHour - this.startHour) * 60 / step_in_minutes);

Doesn't this fit into a single line not exceeding 100 chracters?

Comment 17

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 18

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 19

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 20

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

Comment on attachment 9056288 [details] [diff] [review]
freebusy-day.patch

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

Please re-check documentation for the issues I've brought up before.

Comment 21

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 22

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 23

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 24

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 25

[Arshad Khan [:arshad]]

Attachment: freebusy-day.patch

Comment 26

[Pulsebot]

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