Closed Bug 1523604 Opened 6 years ago Closed 6 years ago

[de-xbl] Remove calendar-subscriptions-richlistbox binding.

Categories

(Calendar :: Lightning Only, enhancement)

Product:
Calendar
Component:
Lightning Only
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(2 files, 3 obsolete files)

cal_subscriptions_dialog.png
(deleted), image/png
Details
calendar-subscriptions-richlistbox.patch
(deleted), patch
arshad
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: tb-war-on-xbl
Assignee: nobody → arshdkhn1
Blocks: 1517040
Attached patch calendar-subscriptions-richlistbox.patch (obsolete) (deleted) — — Splinter Review
Attachment #9041716 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9041716 [details] [diff] [review] calendar-subscriptions-richlistbox.patch Review of attachment 9041716 [details] [diff] [review]: ----------------------------------------------------------------- Where/how can I access this dialog? Please add that to the documentation ::: calendar/base/content/dialogs/calendar-subscriptions-dialog.js @@ +15,5 @@ > +if (!customElements.get("richlistbox")) { > + delete document.createElement("richlistbox"); > +} > + > +class MozCalendarSubscriptionsRichlistbox extends customElements.get("richlistbox") { please add documentation, as in "This widget is typically found at.... and used to ...." @@ +16,5 @@ > + delete document.createElement("richlistbox"); > +} > + > +class MozCalendarSubscriptionsRichlistbox extends customElements.get("richlistbox") { > + addCalendar(aCalendar, bSubscribed) { remove a and b conventions
Attachment #9041716 - Flags: review?(mkmelin+mozilla)

I am not sure here as well, when the Find Calendar option in list-calendar-context-menu(https://searchfox.org/comm-central/source/calendar/base/content/calendar-calendars-list.xul#32) is enabled. Clickin on that opens the dialog which has caleendar-subscription-richlistbox.

So how did you test it? Please find out and document it.

hey when does find calendar option visible(not collapsed) in the popup menu? I have setup caldav calendar for my google account.

Flags: needinfo?(makemyday)

As far I can see, currently only the wcap provider uses that feature, so maybe setting up a fake subscription could trigger enabling that menu item. I'm not aware of any public wcap calendar services. If that doesn't work, you can trigger the dialog that gets opened by the menu item from the error console.

Maybe Philipp's wip patch for caldav calendar discovery that missed the TB60 train might also enable the button. You may want to ping him for a debitrotted patch to check it.

Flags: needinfo?(makemyday)

(In reply to [:MakeMyDay] from comment #7)

As far I can see, currently only the wcap provider uses that feature, so maybe setting up a fake subscription could trigger enabling that menu item. I'm not aware of any public wcap calendar services. If that doesn't work, you can trigger the dialog that gets opened by the menu item from the error console.

Maybe Philipp's wip patch for caldav calendar discovery that missed the TB60 train might also enable the button. You may want to ping him for a debitrotted patch to check it.

Already did, I think he is busy.

Attached patch calendar-subscriptions-richlistbox.patch (obsolete) (deleted) — — Splinter Review

The patch is same as calendar-invitations-richlistbox because the the element is same except the name. I tried hacking a few things and adding richlistitem inside it. The UI to me look ok and the vertical alignment of label and checkbox that you see in the screenshot is not related to this patch.

Attachment #9041716 - Attachment is obsolete: true
Attachment #9045631 - Flags: feedback?(mkmelin+mozilla)
Attached image cal_subscriptions_dialog.png (deleted) —
Attachment #9045631 - Flags: review?(philipp)
Comment on attachment 9045631 [details] [diff] [review] calendar-subscriptions-richlistbox.patch Review of attachment 9045631 [details] [diff] [review]: ----------------------------------------------------------------- I think it makes sense to inline this too, like the other one this is also just used once, and there is not much behavior
Attachment #9045631 - Flags: review?(philipp)
Attachment #9045631 - Flags: feedback?(mkmelin+mozilla)
Attached patch calendar-subscriptions-richlistbox.patch (obsolete) (deleted) — — Splinter Review
Attachment #9045631 - Attachment is obsolete: true
Attachment #9045905 - Flags: review?(philipp)
Attachment #9045905 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9045905 [details] [diff] [review] calendar-subscriptions-richlistbox.patch Review of attachment 9045905 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-subscriptions-dialog.xul @@ +51,5 @@ > value="&calendar.subscriptions.dialog.select.label.value;" > crop="end"/> > </row> > <row flex="1"> > + <richlistbox id="subscriptions-listbox" flex="1"/> NOTE: this will add those scrollbar and opther elements that get appended by richlistbox ce. I dont think they might cause any problem.
Comment on attachment 9045905 [details] [diff] [review] calendar-subscriptions-richlistbox.patch Review of attachment 9045905 [details] [diff] [review]: ----------------------------------------------------------------- Didn't test it, but looks correct to me.
Attachment #9045905 - Flags: feedback?(mkmelin+mozilla) → feedback+
Comment on attachment 9045905 [details] [diff] [review] calendar-subscriptions-richlistbox.patch Review of attachment 9045905 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-subscriptions-dialog.js @@ +102,5 @@ > let richListBox = document.getElementById("subscriptions-listbox"); > + > + while (richListBox.hasChildNodes()) { > + richListBox.lastChild.remove(); > + } You can use this helper: https://searchfox.org/comm-central/source/calendar/base/content/calendar-ui-utils.js#226
Attachment #9045905 - Flags: review?(philipp) → review+

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #15)

You can use this helper:
https://searchfox.org/comm-central/source/calendar/base/content/calendar-ui-utils.js#226

Helpers that do that little should really be killed off asap. That just adds complexity and making it harder for people to work on the coede. It's like having a helper for a for loop. Wait, that's not like - it is a helper for a loop!

Attachment #9045905 - Attachment is obsolete: true
Attachment #9047757 - Flags: review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/42de459b3e60
Migrate calendar-subscriptions-richlistbox binding to custom element. r=philipp

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

Attachment

General

Creator:
Created:
Updated:
Size: