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

RESOLVED FIXED in 7.0

Status

enhancement
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: pmorris, Assigned: pmorris)

Tracking

(Blocks 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 5 obsolete attachments)

Status: NEW → ASSIGNED

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

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

Attachment #9046582 - Attachment is obsolete: true

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

Attachment #9047259 - Attachment is obsolete: true

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.

Attachment #9047260 - Attachment is obsolete: true
Attachment #9054788 - Flags: review?(mkmelin+mozilla)
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.)
Attachment #9054788 - Flags: review?(philipp)
Attachment #9054788 - Flags: review?(mkmelin+mozilla)
Attachment #9054788 - Flags: feedback+

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

Attachment #9054788 - Attachment is obsolete: true
Attachment #9054788 - Flags: review?(philipp)
Attachment #9056165 - Flags: review?(philipp)
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.
Attachment #9056165 - Flags: review?(philipp) → review+

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

Attachment #9056165 - Attachment is obsolete: true

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.

Looks good to me.

Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 7.0
You need to log in before you can comment on or make changes to this bug.