1509180 - [de-xbl] Inline and Scriptify daypicker-monthday binding.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Description

[Arshad Khan [:arshad]]

Binding link - https://searchfox.org/comm-central/source/calendar/base/content/calendar-daypicker.xml#143

Comment 1

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 2

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 3

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 4

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 5

[Arshad Khan [:arshad]]

Comment on attachment 9026848 [details] [diff] [review]
daypicker-monthday.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +53,5 @@
> +            }
> +        }
> +        return days;
> +    },
> +    init: () => {

Using anonymous function here instead of normal function as we dont need `this` binding as in case of daypicker-weekday.

Comment 6

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

try run - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=447b91a7e9df301a29c4b782095411c66747edde

Comment 7

[Arshad Khan [:arshad]]

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

Comment 8

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d0c5fb8a8b60d6cd8aeed4507213fd60ddaf701&selectedJob=213822067 Indeed the test fail was because of ruleaction target binding bug. Now green lights for this patch from test.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9027061 [details] [diff] [review]
daypicker-monthday.patch

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

LGTM

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +32,5 @@
> +        }
> +        for (let i = 0; i < val.length; i++) {
> +            let lastDayOffset = val[i] == -1 ? 0 : -1;
> +            let index = (val[i] < 0 ? val[i] + days.length + lastDayOffset
> +                : val[i] - 1);

copied but very strange use of parenthesis. (I'd put the close after the 0)

Comment 10

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 11

[Arshad Khan [:arshad]]

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

Comment 12

[[:MakeMyDay]]

Comment on attachment 9028071 [details] [diff] [review]
daypicker-monthday.patch

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

This patch contains serveral unrelated (and not wanted) style changes to other parts of the code. Can you please revert them and limit the changes to what is required for the patch?

Comment 13

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 14

[Arshad Khan [:arshad]]

Rebased this patch on top daypicker-weekday and removed the unwanted style changes.

Comment 15

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 16

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 17

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 18

[Arshad Khan [:arshad]]

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

Comment 19

[Arshad Khan [:arshad]]

Comment on attachment 9042960 [details] [diff] [review]
daypicker-monthday.patch

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

try is green.

Comment 20

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

Comment on attachment 9042960 [details] [diff] [review]
daypicker-monthday.patch

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

r+ with these changes:

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +92,5 @@
> +    init() {
> +        let mainbox = document.querySelector(".daypicker-monthday-mainbox");
> +        let numRows = mainbox.childNodes.length;
> +        let child = null;
> +        for (let i = 0; i < numRows; i++) {

Can you go through this patch and convert all relevant for(;;) to for..of since childNodes can now be interated?

@@ +123,5 @@
> +            }
> +        }
> +        for (let i = 0; i < val.length; i++) {
> +            let lastDayOffset = val[i] == -1 ? 0 : -1;
> +            let index = (val[i] < 0) ? (val[i] + days.length + lastDayOffset)

Please check eslint on this if you haven't, I don't think you need the extra parens.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul
@@ +245,5 @@
> +                        class="daypicker-monthday"
> +                        onselect="updateRecurrenceControls();">
> +                    <vbox class="daypicker-monthday-mainbox" flex="1">
> +                      <hbox class="daypicker-row" flex="1">
> +                        <daypicker label="1" mode="monthly-days"/>

This reads a bit strange. Also, I believe custom elements need a - in the name. Can we make this daypicker-day, or something that makes clear that this is a single day instead of a binding that allows selecting a day of many.

Comment 21

[Arshad Khan [:arshad]]

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul
@@ +245,5 @@

•                    class="daypicker-monthday"
  

•                    onselect="updateRecurrenceControls();">
  

•                <vbox class="daypicker-monthday-mainbox" flex="1">
  

•                  <hbox class="daypicker-row" flex="1">
  

•                    <daypicker label="1" mode="monthly-days"/>
  

This reads a bit strange. Also, I believe custom elements need a - in the
name. Can we make this daypicker-day, or something that makes clear that
this is a single day instead of a binding that allows selecting a day of
many.

I am not making any custom element here. I have just scriptfied the js part and inlined the content. daypicker is an xbl element.

Comment 22

[Arshad Khan [:arshad]]

Attachment: daypicker-monthday.patch

Comment 23

[Arshad Khan [:arshad]]

please check in after Bug 1509076.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/da0380b3b4b0
Inline and Scriptify daypicker-monthday binding. r=philipp