Closed Bug 1522473 Opened 6 years ago Closed 6 years ago

[de-xbl] Migrate attendees-list to custom element.

Categories

(Calendar :: General, enhancement)

Product:
Calendar
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 26 obsolete files)

attendees-list.patch (merged)
(deleted), patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Blocks: tb-war-on-xbl
Component: Mail Window Front End → General
Product: Thunderbird → Calendar
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Summary: [de-xbl] Scriptify attendees-list. → [de-xbl] Migrate attendees-list to custom element.
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review

Conversion like this a little bit tricky.. This element uses a method of Richlistbox but only under certain conditon which depends upon layout of the attendees-list..

Attachment #9041708 - Attachment is obsolete: true
Comment on attachment 9041709 [details] [diff] [review] attendees-list.patch Review of attachment 9041709 [details] [diff] [review]: ----------------------------------------------------------------- hey I am adding a lot of css here.. don't know why the xul layout wasn't working.. I tried some css options but this one gives the most appropriate look.
Attachment #9041709 - Flags: ui-review?(richard.marti)
Attachment #9041709 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9041709 [details] [diff] [review] attendees-list.patch Review of attachment 9041709 [details] [diff] [review]: ----------------------------------------------------------------- Only tried briefly but it looks good. ::: calendar/base/themes/common/dialogs/calendar-event-dialog.css @@ +610,5 @@ > + flex: 1; > + display: flex; > +} > + > +#attendees-list hbox:nth-child(1), #attendees-list hbox:nth-child(2) { Please every selector on its own line. @@ +619,5 @@ > +#attendees-list hbox:nth-child(3) { > + flex: 1; > +} > + > +#attendees-list hbox textbox { Instead of this line, could you try: .textbox-addressingWidget {
Attachment #9041709 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 9041709 [details] [diff] [review] attendees-list.patch Review of attachment 9041709 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work, but there are some things that should be cleaned up ::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js @@ +10,5 @@ > +if (!customElements.get("richlistbox")) { > + delete document.createElement("richlistbox"); > +} > + > +class MozAttendeesList extends customElements.get("richlistbox") { Let's add documentation to these as we go, and see which thing is which. Like https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1521016&attachment=9040701 @@ +19,5 @@ > + if (event.button != 0) { > + return; > + } > + > + function cycle(values, current) { need to use let cycle = function(..... ... not to get strict js errors. @@ +81,5 @@ > + } > + if (event.originalTarget.localName == "input") { > + switch (event.key) { > + case "Delete": > + case "Backspace": While we're here case KeyEvent.DOM_VK_DELETE: case KeyEvent.DOM_VK_BACK_SPACE: @@ +133,5 @@ > + return; > + } > + if (event.originalTarget.localName == "input") { > + switch (event.key) { > + case "ArrowUp": these too @@ +172,5 @@ > + this.mPopupOpen = false; > + > + this.mMaxAttendees = 0; > + > + window.addEventListener("load", this.onLoad.bind(this), true); ugh! I don't think this is the sort of thing a custom element should be allowed to do. I wonder if you're in luck though. This is in the (delayed) connectedCallback, so perhaps everything is ready? @@ +226,5 @@ > + return attendees; > + } > + > + get organizer() { > + const { MailServices } = ChromeUtils.import("resource:///modules/MailServices.jsm"); MailServices is bascially universal in mail. You can just put it on top of the file Besides, it's now used in attendees() too, where it would not be defined. @@ +287,5 @@ > + > + // this trigger the continous update chain, which > + // effectively calls this.onModify() on predefined > + // time intervals [each second]. > + let self = this; no need for self here I think, as you're now using => @@ +296,5 @@ > + callback(); > + } > + > + onInitialize() { > + let args = window.arguments[0]; ugh! I don't know why the onLoad and the onInitialize() are split into two. onInitialize() should not be allowed to dig into the window arguments though. I think there should be some outer code that sets the element up with the needed information at the right time. @@ +958,5 @@ > + this.mMaxAttendees--; > + } > +} > + > +customElements.define("attendees-list", MozAttendeesList); perhaps calendar-event-attendees-list?
Attachment #9041709 - Flags: ui-review?
Attachment #9041709 - Flags: ui-review+
Attachment #9041709 - Flags: review?(mkmelin+mozilla)

Oh, and do use hg cp --after, to record the blame.

Attachment #9041709 - Flags: ui-review? → ui-review+

onInitialize() should not be allowed to dig into the window arguments
though. I think there should be some outer code that sets the element up
with the needed information at the right time.

Why not?

Instead of this line, could you try: .textbox-addressingWidget {

This will target every element with this class which I dont want.

Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9041709 - Attachment is obsolete: true
Attachment #9042019 - Flags: ui-review+
Attachment #9042019 - Flags: review?(mkmelin+mozilla)

There are some issues with the css of the whole attendees dialog. fixed position is used which doesn't work well with overflow property. Some elements gets a bit chopped when dialog height is resized. I think it should be fixed in different bug.

(In reply to Arshad Khan [:arshad] from comment #7)

onInitialize() should not be allowed to dig into the window arguments
though. I think there should be some outer code that sets the element up
with the needed information at the right time.

Why not?

That totally destroys the idea that this would be a component that could be reused (well, it's not, but that's a different issue). It leads to magic behaviour that is hard to understand. If I put a component into a page I expect it to be configurable at that place through attributes, or to be configured through global loading code. NOT that it would partly configure itself depending on what the window arguments happen to be, and certainly not somewhere deep inside the basically private methods of the component.

Comment on attachment 9042019 [details] [diff] [review] attendees-list.patch Review of attachment 9042019 [details] [diff] [review]: ----------------------------------------------------------------- Please reflag for feedback once the comments have been addressed.
Attachment #9042019 - Flags: review?(mkmelin+mozilla)
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9042019 - Attachment is obsolete: true
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044492 - Attachment is obsolete: true
Attachment #9044496 - Flags: feedback?(mkmelin+mozilla)
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044496 - Attachment is obsolete: true
Attachment #9044496 - Flags: feedback?(mkmelin+mozilla)
Attachment #9044498 - Flags: feedback?(mkmelin+mozilla)
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044498 - Attachment is obsolete: true
Attachment #9044498 - Flags: feedback?(mkmelin+mozilla)
Attachment #9044501 - Flags: feedback?(mkmelin+mozilla)
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044501 - Attachment is obsolete: true
Attachment #9044501 - Flags: feedback?(mkmelin+mozilla)
Attachment #9044567 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9044567 [details] [diff] [review] attendees-list.patch Review of attachment 9044567 [details] [diff] [review]: ----------------------------------------------------------------- Please hg cp calendar/base/content/dialogs/calendar-event-dialog-attendees.xml calendar/base/content/dialogs/calendar-event-dialog-attendees.js to preserve some blame, and easier review Trying to close the dialog I get JavaScript error: chrome://calendar/content/calendar-event-attendees-custom-elements.js, line 211: ReferenceError: MailServices is not defined There is something wrong with the deletion of rows of invited (using backspace at least) - it doesn't quite work, not "removing the icon" like it used to When loading the dialog I now see a slight visual hickup: the horizontal lines of the grid are showing up after maybe half a seconds delay. I didn't see that before, and not sure if it's due to this patch but thought I'd note it down anyway. ::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/** > + * Class extending RichListBox. AttendeesList is used to add/remove attendee > + * to an event. MozAttendeesList is a widget allowing adding and removing of attendees of an event. It shows if attendee if required or optional, the attendee status, type and adddress. It is typically found in the Invite Attendees dialog. @@ +722,5 @@ > + } > + > + /** > + * Gets the next row from the top down. > + */ there's a bunch of places like this where the documentation indention seems one space off @@ +813,5 @@ > + : '"' + aMsgIAddressObject.name + '"' > + ) + " <" + aMsgIAddressObject.email + ">"; > + } else { > + return aMsgIAddressObject.toString(); > + } while here, fix the else after return (remove the else and just return) ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js @@ +1004,5 @@ > + // assume we're the organizer [in case that the calendar > + // does not support the concept of identities]. > + let organizerID = ((organizer && organizer.id) ? > + organizer.id > + : calendar.getProperty("organizerId")); : on the previous line (I think, unless calendar had something special wrt this) ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xul @@ +31,5 @@ > > <!-- Javascript includes --> > + <script type="application/javascript" > + src="chrome://calendar/content/calendar-event-attendees-custom-elements.js"/> > + <script type="application/javascript" trailing whitespace
Attachment #9044567 - Flags: feedback?(mkmelin+mozilla) → feedback-

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

Comment on attachment 9044567 [details] [diff] [review]
attendees-list.patch

Review of attachment 9044567 [details] [diff] [review]:

Please
hg cp calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
calendar/base/content/dialogs/calendar-event-dialog-attendees.js to preserve
some blame, and easier review

calendar-event-dialog-attendees-custom-elements.js?

@@ +722,5 @@

  • }
  • /**
  • * Gets the next row from the top down.

  • */

there's a bunch of places like this where the documentation indention seems
one space off

how is it off?

(In reply to Arshad Khan [:arshad] from comment #20)

@@ +722,5 @@

  • }
  • /**
  • * Gets the next row from the top down.

  • */

there's a bunch of places like this where the documentation indention seems
one space off

how is it off?

I mean to say that I dont see any other option better than this. I see this identation used everywhere.

Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review

The ui hickup is already there and not caused by this patch. I have fixed the backspace buttona and arrow button issue.

Attachment #9044567 - Attachment is obsolete: true
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9045269 - Flags: feedback?(mkmelin+mozilla)
Attachment #9045263 - Attachment is obsolete: true
Comment on attachment 9045269 [details] [diff] [review] attendees-list.patch Review of attachment 9045269 [details] [diff] [review]: ----------------------------------------------------------------- Seem to be working now. ::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js @@ +888,2 @@ > > +customElements.define("calendar-event-attendees-list", MozAttendeesList); Please make the class correspond to the name. MozCalendarEventAttendeesList
Attachment #9045269 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9045269 - Attachment is obsolete: true
Attachment #9045906 - Flags: feedback+
Attachment #9045269 - Flags: review?(philipp)
Comment on attachment 9045906 [details] [diff] [review] attendees-list.patch Review of attachment 9045906 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js @@ +31,5 @@ > + this.mPopupOpen = false; > + > + this.mMaxAttendees = 0; > + > + this.addEventListener("click", (event) => { Maybe we can move the actual function into a method on the custom element, make sure to bind those methods to the instance on the constructor, and then addEventListener the instance method? This would avoid us doing all the work in the constructor. From a separation perspective, it would also in theory allow to extend this binding and extend the click handler easily. ::: calendar/base/themes/common/dialogs/calendar-event-dialog.css @@ +612,5 @@ > + display: flex; > +} > + > +#attendees-list hbox:nth-child(1), > +#attendees-list hbox:nth-child(2) { Can we give these a class instead of using nth-child?
Attachment #9045906 - Flags: review+
Attached patch part-2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047109 - Flags: review?(philipp)
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047109 - Attachment is obsolete: true
Attachment #9047109 - Flags: review?(philipp)
Attachment #9047750 - Flags: review?(philipp)
Comment on attachment 9047750 [details] [diff] [review] part2-attendees-list-nits.patch Review of attachment 9047750 [details] [diff] [review]: ----------------------------------------------------------------- Removing review until comments are fixed, as discussed via IRC.
Attachment #9047750 - Flags: review?(philipp)
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047750 - Attachment is obsolete: true
Attachment #9047929 - Flags: review?(philipp)
Comment on attachment 9047929 [details] [diff] [review] part2-attendees-list-nits.patch Review of attachment 9047929 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js @@ +910,1 @@ > setFocus(row) { Nog messing with this - https://stackoverflow.com/questions/779379/why-is-settimeoutfn-0-sometimes-useful . This timeout was already there so probably it should be there.
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047929 - Attachment is obsolete: true
Attachment #9047929 - Flags: review?(philipp)
Attachment #9047935 - Flags: review?(philipp)
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047935 - Attachment is obsolete: true
Attachment #9047935 - Flags: review?(philipp)
Attachment #9047951 - Flags: review?(philipp)
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047951 - Attachment is obsolete: true
Attachment #9047951 - Flags: review?(philipp)
Attachment #9047952 - Flags: review?(philipp)
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047952 - Attachment is obsolete: true
Attachment #9047952 - Flags: review?(philipp)
Attachment #9047953 - Flags: review?(philipp)

I haven used {object} instead of {node} at some places so please dont mind that, I ll fix it in next patch.

Comment on attachment 9047953 [details] [diff] [review] part2-attendees-list-nits.patch Review of attachment 9047953 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for all the comments. I tried to explain the concept behind it instead of just flagging it, hope this helps to understand. Let me know if you have questions. ::: calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js @@ +112,5 @@ > > + /** > + * Sets mIsReadOnly property. > + * > + * @param {Boolean} val New mIsReadOnly value When writing jsdoc, consider the class a black box. If you generate this and people only read the generated jsdoc, they won't know what mIsReadOnly means. You should rather describe what it means. The attendee list can be in a readonly state, so you'd have something like "Controls the read-only state of the attendee list, not allowing XYZ when set". The same goes for the other properties. @@ +146,5 @@ > return this.mIsInvitation; > } > > + /** > + * @returns {array} Array of attendee objects for an event These should have a description, whereas the @returns line can be short. Also, with array you rather want to use the type of the array elements and []. I guess it is a calIAttendee? ``` /** * The attendees shown in this attendee list. * * @returns {calIAttendee[]} The attendees of the list. */ ``` Same goes for the others. @@ +268,5 @@ > return val; > } > > + /** > + * Handler for click event. This needs more description, what does the handler do? Not in very much detail, but a high level description. @@ +337,5 @@ > > /** > * Appends a new row using an existing attendee structure. > + * > + * @param {Object} attendee Attendee object Is this an object or calIAttendee? @@ +338,5 @@ > /** > * Appends a new row using an existing attendee structure. > + * > + * @param {Object} attendee Attendee object > + * @param {Object} templateNode Template node that need to be cloned If there is a more specialized type, use it. Using Object is too broad and should only be done if it is a custom object. Custom objects need to be described as well. If this is a DOM element, then you'll want Element. N.B. this shouldn't be Node, since that is the more general type. A Node can be an Element, an attribute node, etc. More details on MDN. Check the remaining file as well. @@ +509,5 @@ > return newNode; > } > > + /** > + * Resolves list by the value that is passed. You are going to have to do more than just describe the name of the function in other words. What does resolving mean here? @@ +557,5 @@ > return null; > } > > + /** > + * Finds attendees in the address lists. This is much better, great! @@ +561,5 @@ > + * Finds attendees in the address lists. > + * > + * @param {Object} mailingList Mailing list having address lists > + * @param {array} attendees List of attendees > + * @param {array} allListsUri URI of all lists Same comment about using type[] instead of array @@ +647,5 @@ > userTypeIcon.setAttribute("cutype", newAttendee.userType); > } > > + /** > + * Resolves list. This needs to be described better @@ +683,5 @@ > } > } > > + /** > + * Emits modify method with list having attendees data. This is the right kind of description for event handlers, great! Emits a |modify| DOM event... @@ +715,5 @@ > this.dispatchEvent(event); > } > > + /** > + * Method setting the tooltip of attendee icons based on their role. This one is great as well! @@ +717,5 @@ > > + /** > + * Method setting the tooltip of attendee icons based on their role. > + * > + * @param {node} targetIcon TargetIcon node Node should be uppercase. Any objects should be uppercase. I've recently learned that only primitive types should be lowercase, I've been doing this wrong in other calendar code. https://github.com/gajus/eslint-plugin-jsdoc#why-not-capital-case-everything As per earlier comment, this should be Element instead if it is a DOM Element. @@ +755,5 @@ > } > } > > + /** > + * Fits dummy rows in the attendee list. Dummy rows isn't very clear, maybe describe what it actually is? I think those were the extra rows that are added to fill out space, so you could say it fills out the empty space in the attendee list with rows that do ... ? @@ +805,5 @@ > } > } > > + /** > + * Creates dummy item. Same here, not sure what a dummy item is? @@ +843,5 @@ > return this.getElementsByTagName("richlistitem")[row - 1]; > } > > + /** > + * @returns {Object} <xul:textbox> at first row The fact that this is a xul:textbox is an implementation detail. Describe what the method does instead. @@ +877,5 @@ > return this.getListItem(row).querySelector(".textbox-addressingWidget"); > } > > + /** > + * @param {Object} row Row element Missing description here. @@ +910,2 @@ > setFocus(row) { > + // See https://stackoverflow.com/questions/779379/why-is-settimeoutfn-0-sometimes-useful to This is a pretty long answer and very generic. You're either going to have to write why it is specifically helpful here, or if you don't know I'm fine to just drop the comment. @@ +925,5 @@ > }, 0); > } > > + /** > + * Creates attendee. ...with default values set. @@ +983,5 @@ > } > } > > + /** > + * Method that gets called when arrow key is pressed. Describe what it does, not when it is called.
Attachment #9047953 - Flags: review?(philipp) → review-
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047953 - Attachment is obsolete: true
Attachment #9050243 - Flags: review?(philipp)

Some of the comments might be wrong. I have tried to write the best I could. TBH while converting an element, I understand the code technically and converts xbl/xul related things into ce stuff.. Many times I dont know some of the methods present and what they do, you guys know the widgets holistically and technically better than me so please guide me when I am wrong. I wont complain how many times I am flagged for re review.

Comment on attachment 9050243 [details] [diff] [review] part2-attendees-list-nits.patch Review of attachment 9050243 [details] [diff] [review]: ----------------------------------------------------------------- Can you apply the review comments from other patches on this first? I see various comments with mTheThing in them.
Attachment #9050243 - Flags: review?(philipp) → review-
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9050243 - Attachment is obsolete: true
Attached patch part2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9056401 - Attachment is obsolete: true
Attachment #9056402 - Flags: review?(philipp)
Comment on attachment 9056402 [details] [diff] [review] part2-attendees-list-nits.patch Review of attachment 9056402 [details] [diff] [review]: ----------------------------------------------------------------- This looks much better than the other patches, keep it up :)
Attachment #9056402 - Flags: review?(philipp) → review+
Attached patch attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9056402 - Attachment is obsolete: true
Attachment #9058180 - Flags: review+

Hi Arshad, looking at
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0bfcfb8422e4a34fcf25be4a49a2d96df40dd961
The X1, X4 failures are expected. If you rebase, the Z4 should be green. Z1, eventDialog/testEventDialogModificationPrompt.js looks like something you caused unless it's intermittent.

Almost ready to go?

Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9045906 - Attachment is obsolete: true
Attachment #9058180 - Attachment is obsolete: true
Attachment #9058815 - Flags: review+
Attached patch part-2-attendees-list-nits.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058816 - Flags: review+
Keywords: checkin-needed

Jorgk, looks like the try is ok?

I'm confused as to what you want landed here. Both patches have the same size, so are they the same?

Th try run doesn't look good since Z2 failed completely, although that may/will be unrelated. The X1 failure has been resolved. So best to do another try. You can add Mac as well as per the e-mail I've sent you on Monday. Add one of these patches to the push:
https://hg.mozilla.org/try-comm-central/rev/ec7d128491d3e1bcc2c5519679d8ee94dec1cdd5
https://hg.mozilla.org/try-comm-central/rev/f680a8802b10c7c365ad360e5a8b607d14432cd0

Keywords: checkin-needed
Attached patch attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058815 - Attachment is obsolete: true
Attachment #9058893 - Flags: review+
Attached patch part-1-attendees-list.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058893 - Attachment is obsolete: true
Attachment #9058894 - Flags: review+

(In reply to Jorg K (GMT+2) from comment #53)

I'm confused as to what you want landed here. Both patches have the same size, so are they the same?

Th try run doesn't look good since Z2 failed completely, although that may/will be unrelated. The X1 failure has been resolved. So best to do another try. You can add Mac as well as per the e-mail I've sent you on Monday. Add one of these patches to the push:
https://hg.mozilla.org/try-comm-central/rev/ec7d128491d3e1bcc2c5519679d8ee94dec1cdd5
https://hg.mozilla.org/try-comm-central/rev/f680a8802b10c7c365ad360e5a8b607d14432cd0

Sorry I uploaded same patch. I have fixed that now. Also I have pushed another try run with first patch - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c330d5c396af8ea16399c024b73e062329ae5515

Keywords: checkin-needed
Attached patch attendees-list.patch (merged) (deleted) — — Splinter Review

Merged patch. What you've done here made no sense. In the first patch you did:

copy from calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
copy to calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js

which is not a good idea to start with since and XML file is not a JS file. Moving a file pr preserve history might be OK. However, the first patch doesn't make any changes to the new file calendar-event-attendees-custom-elements.js.

In the second patch, you delete the new file again:
deleted file mode 100644
--- a/calendar/base/content/dialogs/calendar-event-attendees-custom-elements.js

So that's really confusing and unnecessary churn. I've folded the two patches. Now that stuff that is removed from the XML file goes into an existing custom elements JS file.

Attachment #9058816 - Attachment is obsolete: true
Attachment #9058894 - Attachment is obsolete: true
Attachment #9059027 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1552305b49b0
Migrate attendees-list binding to custom-element. r=philipp

Status: ASSIGNED → RESOLVED
Closed: 6 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: