1512943 - [de-xbl] convert timepicker-hour to custom element.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Comment 1

[Arshad Khan [:arshad]]

Attachment: timepicker-hour.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9030414 [details] [diff] [review]
timepicker-hour.patch

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

The patch doesn't include the removal of the binding

::: common/bindings/datetimepicker.js
@@ +83,5 @@
> +            return;
> +        }
> +
> +        if (this.hasAttribute("label")) {
> +            this.label.setAttribute("value", this.getAttribute("label"));

you're not handling the removal of label here (same below)

::: mail/base/content/customElements.js
@@ +10,5 @@
>    "chrome://messenger/content/mailWidgets.js",
>    "chrome://messenger/content/generalBindings.js",
>    "chrome://messenger/content/statuspanel.js",
>    "chrome://messenger/content/foldersummary.js",
> +  "chrome://messenger/content/datetimepicker.js",

belongs in lighthing

::: mail/base/jar.mn
@@ +38,5 @@
>  *   content/messenger/viewSource.xul                (../../common/src/viewSource.xul)
>  *   content/messenger/datetimepicker.xml            (../../common/bindings/datetimepicker.xml)
>      content/messenger/generalBindings.xml           (../../common/bindings/generalBindings.xml)
>      content/messenger/generalBindings.js            (../../common/bindings/generalBindings.js)
> +    content/messenger/datetimepicker.js             (../../common/bindings/datetimepicker.js)

this too, belongs in lightning

Comment 3

[Arshad Khan [:arshad]]

Attachment: timepicker-hour.patch

This patch should be applied on top of timepicker-minute patch.

Comment 4

[Arshad Khan [:arshad]]

See Bug 1512941 for the timepicker-hour patch changes.

Comment 5

[Arshad Khan [:arshad]]

Attachment: timepicker-hour.patch

Comment 6

[Arshad Khan [:arshad]]

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

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9047948 [details] [diff] [review]
timepicker-hour.patch

(this is not reviewed yet)

Comment 8

[Arshad Khan [:arshad]]

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

Comment on attachment 9047948 [details] [diff] [review]
timepicker-hour.patch

(this is not reviewed yet)

Fallen reviewed it in https://bugzilla.mozilla.org/show_bug.cgi?id=1512941#c24

Comment 9

[Arshad Khan [:arshad]]

Attachment: timepicker-hour.patch

is it ok now?

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9047957 [details] [diff] [review]
timepicker-hour.patch

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

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +185,5 @@
> +
> +        disconnectedCallback() {
> +            while (this.hasChildNodes()) {
> +                this.firstChild.removeChild();
> +            }

I think it's preferable to instead in connectedCallback() check this.hasChildNodes(), and if there are, don't add again

Comment 11

[Arshad Khan [:arshad]]

Attachment: timepicker-hour.patch

Comment 12

[Arshad Khan [:arshad]]

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

Comment 13

[Arshad Khan [:arshad]]

Attachment: timepicker-hour.patch

Comment 14

[Jorg K (CEST = GMT+2)]

Hmm, did you check your try before requesting checkin?

Comment 15

[Arshad Khan [:arshad]]

(In reply to Jorg K (GMT+1) from comment #14)

Hmm, did you check your try before requesting checkin?
https://bugzilla.mozilla.org/show_bug.cgi?id=1512943#c12

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c099a18f37b3
Convert timepicker-hour binding to custom element. r=philipp DONTBUILD

Comment 17

[Jorg K (CEST = GMT+2)]

A few remarks:
The patch provided here did NOT apply. It had a hunk

-                let makeFormatRegExp = function (string) {
+                let makeFormatRegExp = function(string) {

which seemed to have moved into the patch from bug 1512942.

Also, would you mind including the word "binding" in your commit messages, I add it every time. Have you never noticed?
Lastly, https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch says that the commit message should look like this:
Bug 123456 - Change this thing to work better by doing something. r=reviewers
so no colon or semicolon.

As for the reviewer. You can used the IRC name, for Philipp that would be "Fallen" (uppercase), but Philipp's colleague MMD consistently uses "philipp". This has been discussed before, and Philipp said that he's OK with both.