Closed
Bug 1477478
Opened 6 years ago
Closed 6 years ago
The event attendees dialog contains three listboxes
Categories
(Calendar :: Dialogs, enhancement)
Calendar
Dialogs
Tracking
(Not tracked)
RESOLVED
FIXED
6.5
People
(Reporter: darktrojan, Assigned: darktrojan)
References
Details
Attachments
(1 file, 4 obsolete files)
|
74.57 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
As we all know, listboxes aren't a thing any more. I thought I'd better file a bug on this before somebody else wastes more time fixing it. Warning: monster patch incoming, when I've finished it.
|
||
Comment 1•6 years ago
|
||
Yes, I've convert the attendees-lsit already here already, fb is a little more challenging. However, I'm fine if you fix it, but keep it simple for the bustage fix - we need to rework the whole thing anyway. There is bug 995746 on file for it.
|
Assignee | |
Comment 2•6 years ago
|
||
I can't count.
Summary: The event attendees dialog contains two listboxes → The event attendees dialog contains three listboxes
|
|
Assignee | |
Comment 3•6 years ago
|
||
I wasn't going to post this tonight, but since you've commented, MMD. You probably know this dialog way better than I do, so you can see what still needs to be done.
> keep it simple for the bustage fix
Hahaha… things got a little… out of hand. I had a fight with XBL. :-)
Attachment #8993908 -
Flags: feedback?(makemyday)
|
|
||
Comment 4•6 years ago
|
||
Comment on attachment 8993908 [details] [diff] [review] 1477478-attendees-1.diff Review of attachment 8993908 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good already and restores most of the previous behaviour (including some predating shortcomings like the not exact match of grid header and boxes, but we leave them beside for this bug). Bug 1447830 landed already, so you need to adapt your patch a little to still apply. Remaining issue: Freebusy lookup results are not displayed (you can only observe this if you have a server at hand which support fb requests - result is always displayed as unknown). This is due to the removal of XBL-inherit="range" in the freebusy-row definition. You can e.g. replace |this.mRange = Number(this.getAttribute("range"));| by |this.mRange = document.querySelector("#freebusy-grid").getAttribute("range");| in the constructor of the freebusy-row binding to resolve this. ::: calendar/base/themes/common/dialogs/calendar-event-dialog.css @@ +455,5 @@ > + height: var(--spacer-top-height); > +} > + > +.attendee-spacer-bottom { > + height: var(--spacer-bottom-height); Can you add a comment that these vars are defined at run-time?
Attachment #8993908 -
Flags: feedback?(makemyday) → feedback+
|
|
Assignee | |
Comment 5•6 years ago
|
||
Thanks for that, MakeMyDay. I'd agree that this dialog needs an overhaul, hopefully these changes have made it a bit more resilient in the meantime.
Attachment #8993908 -
Attachment is obsolete: true
Attachment #8994087 -
Flags: review?(philipp)
|
|
Assignee | |
Comment 6•6 years ago
|
||
Update because m-c removed another richlistbox method we used here.
Attachment #8994087 -
Attachment is obsolete: true
Attachment #8994087 -
Flags: review?(philipp)
Attachment #8995397 -
Flags: review?(philipp)
|
||
Comment 7•6 years ago
|
||
Comment on attachment 8995397 [details] [diff] [review] 1477478-attendees-3.diff Review of attachment 8995397 [details] [diff] [review]: ----------------------------------------------------------------- Just a few suggestions, I'll also accept if you don't take care of them: ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml @@ +750,5 @@ > > <method name="getRoleElement"> > <parameter name="aRow"/> > <body><![CDATA[ > + return document.getElementById("attendeeCol1#" + aRow); I never liked that we are using ids here. Any chance we can use querySelector relative to the binding? If its not a quick fix we can do it another day (tm) ::: calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml @@ +1318,2 @@ > for (let row = aRow + 1; row <= max; row++) { > + let colID = "attendeeCol" + col + "#" + row; I know this was the same as before, but you could make it all snazzy with |let colID = `attendeeCol${col}#${row}`;|
Attachment #8995397 -
Flags: review?(philipp) → review+
|
|
Assignee | |
Comment 8•6 years ago
|
||
With pleasure!
Attachment #8995397 -
Attachment is obsolete: true
Attachment #8997225 -
Flags: review?(philipp)
|
|
Assignee | |
Comment 9•6 years ago
|
||
Oops, left some unused vars behind.
Attachment #8997225 -
Attachment is obsolete: true
Attachment #8997225 -
Flags: review?(philipp)
Attachment #8997226 -
Flags: review?(philipp)
|
|
||
Comment 10•6 years ago
|
||
Comment on attachment 8997226 [details] [diff] [review] 1477478-attendees-5.diff Review of attachment 8997226 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/themes/common/dialogs/calendar-event-dialog.css @@ +454,5 @@ > padding-inline-start: 8px; > padding-inline-end: 10px; > } > > +/* --spacer-top-height and --spacer-bottom-height are defined at runtime */ I'm wondering if we should define a default value for in case it wasn't defined yet, I think var() has a second parameter for that?
Attachment #8997226 -
Flags: review?(philipp) → review+
|
|
Assignee | |
Comment 11•6 years ago
|
||
I think it's okay. If everything works as intended it's called at the window load event, and if not, we have bigger problems than the spacer being the wrong size.
|
|
Assignee | |
Comment 12•6 years ago
|
||
This should be the last of the listboxes, unless I've forgotten something. R.I.P. listbox.
Keywords: checkin-needed
|
||
Comment 13•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/0b7261b6e5e5 Remove listboxes from event attendees dialog. r=philipp
|
||
Updated•6 years ago
|
Target Milestone: --- → 6.5
You need to log in
before you can comment on or make changes to this bug.
Description
•