Closed Bug 1229141 Opened 4 years ago Closed 4 years ago

Use new itip icons in event summary dialog

Categories

(Calendar :: Dialogs, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached image summary.png
This bug is about making use of the new itip icons from bug 1197623 in the summary dialog. I have already a WIP patch, the screenshots illustrates the look how it looks.

The patch does not yet comprise the tooltip composing as in bug 1174511 but keeps the existing approach. Depending on when the patch is ready for review with respect to string freeze, this still can be added.
Patch for review.

The tooltip include also the descriptions to reflect the attendee information now. This patch requires the one from bug 515347 to work.

There is a known issue with the dialog resizing across closing and reopening, but this is bug 1229155.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8696231 - Flags: ui-review?(richard.marti)
Attachment #8696231 - Flags: review?(philipp)
Wrong bug reference for the required sendond patch - not bug 515347 but 1197623.
Blocks: 1229148
When I'm the organizer and invite two attendees, one required and one optional, both not confirmed, I get this in the summary dialog: the organizer (me) is unconfirmed and no attendee visible. Is this expected like this?

On the right is the dialog in writable mode.
No, it's not of cause. This is indicating a js error in the dialog processing. Based on the screenshot, you've applied also the patch from bug 1229148 - did the problem in the summary dialog also occur without the other patch applied?
Removed bug 1229148 but no difference in display. I also removed all events and tasks to have a clean calendar. On opening the event in read only mode I get this:

Sat Dec 05 2015 19:24:01
Error: TypeError: attendee is undefined
Source file: chrome://calendar/content/calendar-summary-dialog.js
Line: 131

Also the mouse pointer changes to an hour glass. I can click all with this cursor. Only when changing to a other tab the cursor changes to normal.
Updated patch fixing the issue Richard observed and also a style issue with the icon max-height.

As discussed on IRC, hightlighting single attendees is not trivial in this implementation approach and may be left for a follow up patch.
Attachment #8696231 - Attachment is obsolete: true
Attachment #8696231 - Flags: ui-review?(richard.marti)
Attachment #8696231 - Flags: review?(philipp)
Attachment #8696283 - Flags: ui-review?(richard.marti)
Attachment #8696283 - Flags: review?(philipp)
Comment on attachment 8696283 [details] [diff] [review]
UseNewItipIconsInSummaryDialog-V2.diff

Review of attachment 8696283 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with the color changes ui-r+

::: calendar/base/themes/common/calendar-attendees.css
@@ +7,5 @@
> +
> +#item-attendees-box {
> +    margin: 0px 4px;
> +    border: solid #cccccc 1px;
> +    background-color: #ffffff;

It would be better when you use systemcolors instead hard coded colors. Like ThreeDShadow for the border and -moz-field for the background-color.

Then the items are also visible on dark HC themes.
Attachment #8696283 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8696283 [details] [diff] [review]
UseNewItipIconsInSummaryDialog-V2.diff

Review of attachment 8696283 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, r+ with the following comments:

::: calendar/base/content/dialogs/calendar-dialog-utils.js
@@ +525,5 @@
> +                                 .cloneNode(true);
> +        let clonedSpacer = document.getElementById("item-attendees-box-template")
> +                                   .getElementsByClassName("item-attendees-row")[0]
> +                                   .getElementsByTagName("box")[1]
> +                                   .cloneNode(false);

You can probably use document.querySelector('#item-attendees-box-template .item-attendees-row box:nth-of-type(2)') here, or consider giving that box a more specific class name.

Similar for the other two variables.

@@ +528,5 @@
> +                                   .getElementsByTagName("box")[1]
> +                                   .cloneNode(false);
> +
> +        // determining of attendee box setup
> +        let inRow = window.inRow || -1;

Maybe use something more elaborate for the window variable, e.g. window.attendeesInRow.

@@ +536,5 @@
> +        } else {
> +            let prevInRow = attBox.getElementsByClassName("item-attendees-row")[0].childNodes.length;
> +            if (prevInRow != window.inRow) {
> +                while(attBox.getElementsByClassName("item-attendees-row").length > 0) {
> +                    attBox.removeChild(attBox.getElementsByClassName("item-attendees-row")[0]);

This would get elements by classname twice in each loop iteration. Can you put them into a local variable once and then loop through those?

@@ +572,5 @@
> +                text.setAttribute("value", label);
> +                icon.setAttribute("partstat", ps);
> +                icon.setAttribute("usertype", ut);
> +                icon.setAttribute("role", role);
> +                cell.setAttribute("id", attendee.id);

I'd suggest prefixing the id with something here.

@@ +589,5 @@
> +
> +                attCount++;
> +            }
> +            // we fill the row with placeholders if required
> +            while (attBox.getElementsByClassName("item-attendees-row").length > 1 &&

Again getting elements once per loop iteration. The number of item-attendees-row elements doesn't look like it will get smaller during the loop, maybe you can put this together with inRow > 1 into an if() around the while().

::: calendar/base/themes/common/calendar-attendees.css
@@ +9,5 @@
> +    margin: 0px 4px;
> +    border: solid #cccccc 1px;
> +    background-color: #ffffff;
> +    overflow-y: auto;
> +    min-height: 23px; /*at least one row*/

Might make sense to specify this in em units

@@ +13,5 @@
> +    min-height: 23px; /*at least one row*/
> +}
> +
> +#item-attendees-box [dialog-type="summary"] {
> +    max-height: 90px; /* displays up to four rows of attendees*/

same here, and maybe some other rules

::: calendar/locales/en-US/chrome/calendar/calendar.properties
@@ +160,5 @@
> +# LOCALIZATION_NOTE(dialog.tooltip.attendeeRole.NON-PARTICIPANT): tooltip for itip icon in summary
> +# and event dialog
> +# %1$S - value of dialog.tooltip.attendeeUserType.*
> +# %2$S - value of dialog.tooltip.attendeePartStat.*
> +dialog.tooltip.attendeeRole.NON-PARTICIPANT=%1$S is a non participant and %2$S.

%1$S is a non-participant and %2$S.
Attachment #8696283 - Flags: review?(philipp) → review+
All comments from comments #7 and #8 considered.

https://hg.mozilla.org/comm-central/rev/263e3488677a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
Typo in patch: RESSOURCE --> RESOURCE
Depends on: 1235355
Alfred, thanks for catching this. I've filed bug 1235355 for this.
You need to log in before you can comment on or make changes to this bug.