Closed
Bug 1509180
Opened 6 years ago
Closed 6 years ago
[de-xbl] Inline and Scriptify daypicker-monthday binding.
Categories
(Calendar :: Lightning Only, enhancement)
Calendar
Lightning Only
Tracking
(Not tracked)
RESOLVED
FIXED
6.9
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 10 obsolete files)
|
(deleted),
patch
|
arshad
:
review+
|
Details | Diff | Splinter Review |
|
Assignee | |
Updated•6 years ago
|
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
|
|
Assignee | |
Updated•6 years ago
|
Blocks: tb-war-on-xbl
|
|
Assignee | |
Comment 1•6 years ago
|
||
Attachment #9026836 -
Flags: review?(mkmelin+mozilla)
Attachment #9026836 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Comment 2•6 years ago
|
||
Attachment #9026836 -
Attachment is obsolete: true
Attachment #9026836 -
Flags: review?(mkmelin+mozilla)
Attachment #9026836 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9026840 -
Flags: review?(makemyday)
Attachment #9026840 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Comment 3•6 years ago
|
||
Attachment #9026840 -
Attachment is obsolete: true
Attachment #9026840 -
Flags: review?(makemyday)
Attachment #9026840 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9026841 -
Flags: review?(makemyday)
Attachment #9026841 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Comment 4•6 years ago
|
||
Attachment #9026841 -
Attachment is obsolete: true
Attachment #9026841 -
Flags: review?(makemyday)
Attachment #9026841 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9026848 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Comment 5•6 years ago
|
||
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.
|
|
Assignee | |
Comment 6•6 years ago
|
||
Attachment #9026848 -
Attachment is obsolete: true
Attachment #9026848 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Comment 7•6 years ago
|
||
|
|
Assignee | |
Updated•6 years ago
|
Attachment #9027061 -
Flags: review?(makemyday)
Attachment #9027061 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Comment 8•6 years ago
|
||
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•6 years ago
|
||
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)
Attachment #9027061 -
Flags: feedback?(mkmelin+mozilla) → feedback+
|
|
Assignee | |
Comment 10•6 years ago
|
||
Attachment #9027061 -
Attachment is obsolete: true
Attachment #9027061 -
Flags: review?(makemyday)
Attachment #9028071 -
Flags: review?(makemyday)
Attachment #9028071 -
Flags: feedback+
|
|
Assignee | |
Comment 11•6 years ago
|
||
|
||
Comment 12•6 years ago
|
||
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?
|
|
Assignee | |
Comment 13•6 years ago
|
||
Attachment #9028071 -
Attachment is obsolete: true
Attachment #9028071 -
Flags: review?(makemyday)
Attachment #9029909 -
Flags: review?(makemyday)
Attachment #9029909 -
Flags: feedback+
|
|
Assignee | |
Comment 14•6 years ago
|
||
Rebased this patch on top daypicker-weekday and removed the unwanted style changes.
Depends on: 1509076
|
|
Assignee | |
Comment 15•6 years ago
|
||
Attachment #9029909 -
Attachment is obsolete: true
Attachment #9029909 -
Flags: review?(makemyday)
Attachment #9029910 -
Flags: review?(makemyday)
Attachment #9029910 -
Flags: feedback+
|
|
Assignee | |
Comment 16•6 years ago
|
||
Attachment #9029910 -
Attachment is obsolete: true
Attachment #9029910 -
Flags: review?(makemyday)
Attachment #9029911 -
Flags: review?(makemyday)
Attachment #9029911 -
Flags: feedback+
|
|
Assignee | |
Comment 17•6 years ago
|
||
Attachment #9029911 -
Attachment is obsolete: true
Attachment #9029911 -
Flags: review?(makemyday)
Attachment #9042960 -
Flags: review?(makemyday)
|
|
Assignee | |
Comment 18•6 years ago
|
||
|
|
Assignee | |
Comment 19•6 years ago
|
||
Comment on attachment 9042960 [details] [diff] [review]
daypicker-monthday.patch
Review of attachment 9042960 [details] [diff] [review]:
-----------------------------------------------------------------
try is green.
Attachment #9042960 -
Flags: review?(makemyday) → review?(geoff)
|
||
Comment 20•6 years ago
|
||
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.
Attachment #9042960 -
Flags: review?(geoff) → review+
|
|
Assignee | |
Comment 21•6 years ago
|
||
::: 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.
|
|
Assignee | |
Comment 22•6 years ago
|
||
Attachment #9042960 -
Attachment is obsolete: true
Attachment #9047898 -
Flags: review+
|
|
Assignee | |
Updated•6 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 23•6 years ago
|
||
please check in after Bug 1509076.
|
||
Comment 24•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/da0380b3b4b0
Inline and Scriptify daypicker-monthday binding. r=philipp
|
||
Updated•6 years ago
|
Target Milestone: --- → 6.9
You need to log in
before you can comment on or make changes to this bug.
Description
•