1530503 - [de-xbl] convert calendar-alarm-widget to custom element

Status: RESOLVED FIXED

(Calendar :: Alarms, enhancement)

Description

[Paul Morris [:pmorris]]

https://searchfox.org/comm-central/source/calendar/base/content/widgets/calendar-alarm-widget.xml#17

Comment 1

[Paul Morris [:pmorris]]

Attachment: calendar-alarm-widget-de-xbl-0.patch

This patch is on top of the changes in bug 1528201, so I am defering requesting a review until the other patch lands.

Comment 2

[Paul Morris [:pmorris]]

Attachment: calendar-alarm-widget-de-xbl-1.patch

A couple of small minor changes, for greater consistency with the way other custom element JS files are organized.

Comment 3

[Paul Morris [:pmorris]]

Attachment: calendar-alarm-widget-de-xbl-2.patch

Oops, here's the right file with the minor changes.

Comment 4

[Paul Morris [:pmorris]]

Attachment: calendar-alarm-widget-de-xbl-3.patch

This patch is on top of the most recent patch in bug 1528201 (calendar-snooze-popup-6.patch). Hopefully that bug will be done soon, so I'm going ahead and requesting review here, but feel free to hold off on reviewing for now, if you prefer.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9054788 [details] [diff] [review]
calendar-alarm-widget-de-xbl-3.patch

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

Needs a proper commit message

Other than that, seems good!

::: calendar/base/content/widgets/calendar-alarm-widget.js
@@ +9,5 @@
>  // Wrap in a block to prevent leaking to window scope.
>  {
>      /**
> +     * Represents an alarm in the alarms dialog. It appears there when an alarm is fired, and
> +     * allows the alarm to be snoozed, dismissed, etc.

please add @extends
That could link nicely in generated documentation

@@ +205,5 @@
> +            relativeDateLabel.textContent = relativeDateString;
> +        }
> +
> +        /**
> +         * Click/keypress handler for "Details" link. Dispatches an event to open an item dialog.

I'll just note that the details link opens the event details dialog opens behind the dialog where the link is in. (Didn't check if this is the case on trunk too.)

Comment 6

[Paul Morris [:pmorris]]

Attachment: calendar-alarm-widget-de-xbl-4.patch

Okay, now with proper commit message, @extends, and some comment formatting (capitalization, punctuation). And rebased on top of latest snooze popup patch (#8).

Comment 7

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

Comment on attachment 9056165 [details] [diff] [review]
calendar-alarm-widget-de-xbl-4.patch

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

Could you hg move -A calendar-alarm-widget.xml calendar-alarm-widgets.js ? I believe this will help preserve some history, but maybe you've already done that.

Comment 8

[Paul Morris [:pmorris]]

Attachment: calendar-alarm-widget-de-xbl-5.patch

To preserve history I've combined this patch with the related calendar snooze popup patch from bug 1528201. That way the old XBL binding file is copied to the new JS file all at once in one patch.

Try server run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6197a66634b17d9ef901fbe0d31527e923bf4a72

Comment 9

[Paul Morris [:pmorris]]

Looks like none of the test failures on the try server run are calendar tests, so I think this is ready to land.

Since I'm new at reading the try server results, someone with more experience might want to have a second look, just for good measure.

Comment 10

[Magnus Melin [:mkmelin]]

Looks good to me.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b8523a5ed09c
[de-xbl] convert calendar-alarm-widget binding to custom element. r=philipp