[de-xbl] removal/conversion of agenda-list-bindings (used in today pane): agenda-base-richlist-item, agenda-checkbox-richlist-item, agenda-richlist-item
Categories
(Calendar :: General, enhancement)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: khushil324)
References
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
de-XBL the bindings used for the richlistbox in the Today pane: agenda-base-richlist-item, agenda-checkbox-richlist-item, agenda-richlist-item
https://searchfox.org/comm-central/source/calendar/base/content/agenda-listbox.xml#11
Their methods are called from agenda-listbox.js, a
https://searchfox.org/comm-central/source/calendar/base/content/agenda-listbox.js
It's possible to convert these items to Custom Element, but given that they don't have too much behaviour, and are used in a very narrow way, it's seems we can just move the functionality to be handled inside agenda-listbox.js
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
I have converted all the bindings to custom elements, did not move it to agenda-listbox because I thought it will make things hard to understand.
Reporter | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Reporter | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Reporter | ||
Comment 6•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 7•6 years ago
|
||
I am getting this error on eslint in agenda-listbox.js : Identifier name 'is' is too short (< 3). id-length (eslint)
when doing this : document.createElement("richlistitem", { is: "agenda-allday-richlist-item" });
Any suggestions ?
Comment 8•6 years ago
|
||
I would just disable eslint for that line. I think something like this should do it:
// eslint-disable-next-line id-length
document.createElement("richlistitem", { is: "agenda-allday-richlist-item" });
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Paul Morris [:pmorris] from comment #8)
I would just disable eslint for that line. I think something like this should do it:
// eslint-disable-next-line id-length
document.createElement("richlistitem", { is: "agenda-allday-richlist-item" });
Thanks. It worked.
Assignee | ||
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
Glad that worked. I've compared my latest patch from bug 1534382 with the latest one here. Here are the differences I thought were worth mentioning:
-
double clicks: I added an event handler to prevent double clicks from opening two dialogs -- a dialog for editing the task double clicked on, and a dialog creating a new task. I haven't tested Khushil's patch to see if this is happening, but it's worth checking.
// Prevent double clicks from opening a dialog to create a new event. this.addEventListener("dblclick", (event) => { event.stopPropagation(); });
-
file names: I moved agenda-listbox.js to agenda-listbox-utils.js, and then made agenda-listbox.xml into the new agenda-listbox.js. I think this is a bit clearer than having those two files named agenda-list.js and agenda-listbox.js. It makes their relationship easier to understand.
-
delayConnectedCallbacks: I noticed there were no
if (this.delayConnectedCallback())
checks in two of the connectedCallbacks. I thought we were always including those. -
event listeners: I had put event listeners in the constructor, rather than in connectedCallback, which seems to be the recommended place to put them, from what I've read.
Updated•6 years ago
|
Reporter | ||
Comment 12•6 years ago
|
||
(In reply to Paul Morris [:pmorris] from comment #11)
- file names: I moved agenda-listbox.js to agenda-listbox-utils.js, and then
made agenda-listbox.xml into the new agenda-listbox.js. I think this is a
bit clearer than having those two files named agenda-list.js and
agenda-listbox.js. It makes their relationship easier to understand.
True
- delayConnectedCallbacks: I noticed there were no
if (this.delayConnectedCallback())
checks in two of the connectedCallbacks. I
thought we were always including those.
I think we should yes.
- event listeners: I had put event listeners in the constructor, rather than
in connectedCallback, which seems to be the recommended place to put them,
from what I've read.
I read that somewhere too, but on the other hand it's good to delay setup as long as possible. Also especially for click handlers, any inline onclicks won't get the expected priority otherwise (should such exist). So I'm not really convinced.
Khuhil, can you look into the points mentioned by Paul, and update your patch accordingly, or then we could also go with Paul's patch.
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 13•6 years ago
|
||
Yes, I will update the patch.
Comment 14•6 years ago
|
||
(In reply to Khushil Mistry [:khushil324] from comment #13)
Yes, I will update the patch.
Oh, yes, better to update your patch rather than use mine. There were a number of places where yours was more polished. I just didn't mention them because I was focusing on things to add or change.
Assignee | ||
Comment 15•6 years ago
|
||
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Paul Morris [:pmorris] from comment #11)
double clicks: I added an event handler to prevent double clicks from opening two dialogs -- a dialog for editing the task double clicked on, and a dialog creating a new task. I haven't tested Khushil's patch to see if this is happening, but it's worth checking.
// Prevent double clicks from opening a dialog to create a new event. this.addEventListener("dblclick", (event) => { event.stopPropagation(); });
I didn't find any problem with this. It was opening single dialog on double clicking.
Reporter | ||
Comment 17•6 years ago
|
||
Double clicking an event in the Today Pane I get the double dialog problem Paul mentions
Reporter | ||
Comment 18•6 years ago
|
||
Assignee | ||
Comment 19•6 years ago
|
||
I have added EventListener for double click to stop propagation in agenda-allday-richlist-item and agenda-richlist-item.
Reporter | ||
Comment 20•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 21•6 years ago
|
||
Assignee | ||
Comment 22•6 years ago
|
||
Assignee | ||
Comment 23•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 25•6 years ago
|
||
Sorry, but your try has failures:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/testTodayPane.js | testTodayPane.js::testTodayPane
Assignee | ||
Comment 27•6 years ago
|
||
Comment 28•6 years ago
|
||
Assignee | ||
Comment 29•6 years ago
|
||
Last time, one of the test, testTodayPane failed. So I have updated that test in the latest patch. It passed but I am seeing lots of tests with orange so I am not sure why that is happening.
Comment 30•6 years ago
|
||
Well, all the opt failures are expected, no idea about the debug stuff. Let's take it like this.
Comment 31•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f57c1c1b7400
[de-xbl] convert agenda-list-bindings to custom elements. r=philipp
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Description
•