498587 - Invite Attendees dialog: add tooltips to attendee role icons

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Stefan Sitter [:ssitter]]

Follow up for Bug 395281

Bug 395281 added a legend below the free-busy-grid to explain the attendee role icons. In addition a tooltip with the same text should be added to the icon that is displayed next to the attendee name.

Comment 1

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

This should be a quite simple bug to start out with

Comment 2

[m.zahnlecker]

Attachment: Funktional edits here, also needed property file

Added new function to update and etablish a tooltip for the icons. Function is called on creation and modification of a list item.
Because of localization a property file is needed. Strings could be taken from calendar-event-dialog.dtd

Comment 3

[m.zahnlecker]

Attachment: Mentioned property file

Comment 4

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: [medium,l10n] Fix - v1

Since our docs are outdated, here the updated hg patch. I'll review later on, there are some issues to be taken care of. (i.e use calGetString() instead of the string bundle directly)

Comment 5

[Decathlon]

I think should be set a dependency with bug 534045 because if patch attached there is correct, an icon "NON-PARTICIPANT" should be added to the set of role icons, hence some code should be added to this patch or to the other one depending on which one is reviewed (and checked in) first.

Comment 6

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 418836 [details] [diff] [review]
[medium,l10n] Fix - v1 

Moving review to mschroeder, so he can decide what to do with the previous comment.

Comment 7

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 418836 [details] [diff] [review]
[medium,l10n] Fix - v1 

>+        
>+        //Set tooltip for rolenames
Add a space ^ here

>+     <method name="updateTooltip">
>+      <parameter name="targetIcon"/>
>+      <body><![CDATA[
>+        // Function setting the tooltip of attendeeicons based on their role
>+        if (targetIcon.hasAttribute("role")) {
Checking can no longer be dependant on the role. Rather check targetIcon.className == "role-icon"


>+            if (targetIcon.hasAttribute("disabled") &&
>+                targetIcon.getAttribute("disabled")) {
>+                return;
>+            }
Why should the disabled icon not have an updated tooltip?


>+
>+            switch (role) {
>+                case "REQ-PARTICIPANT":
>+                    attendeeRoleName = "event.attendee.role.required";
>+                    break;
>+                case "OPT-PARTICIPANT":
>+                    attendeeRoleName = "event.attendee.role.optional";
>+                    break;
>+                case "CHAIR":
>+                    attendeeRoleName = "event.attendee.role.chair";
>+                    break;
>+                default:
>+                    attendeeRoleName = "Not defined";
>+                    break;
>+            }

instead of using a switch, you could use a constant map, i.e:

const strMap = {
  "REQ-PARTICIPANT": "required",
  "OPT-PARTICIPANT": "optional",
  "NON-PARTICIPANT": "nonparticipant",
  "CHAIR": "chair"
}

roleNameString = "event.attendee.role." + (role in strMap ? strMap[role] : "unknown");


>+
>+            if (attendeeRoleName) {
>+                targetIcon.setAttribute("tooltiptext", attendeeStrings.GetStringFromName(attendeeRoleName));
>+            }
Since you have a default case, you don't have to check the if() part. Also, we have some helpers:

let tooltip = cal.calGetString("calendar-event-dialog-attendees", roleNameString);
targetIcon.setAttribute("tooltiptext", tooltip);
                        

>+#
>+# The Initial Developer of the Original Code is
>+#   Your Name Here <[e-mail]>
Please Fill in your name here.



>+event.attendee.role.required=Required Attendee
>+event.attendee.role.optional=Optional Attendee
>+event.attendee.role.chair=Chair
We should also have a string for unknown roles. Either you can take a simple string, i.e "Unknown Attendee", or you can modify the code to include the actual role string, i.e "Unknown Attendee (%1$S)" (which would expand to: "Unknown Attendee (X-FOO-ROLE)".


r- to fix the above issues and to get the patch up to date.

Comment 8

[m.zahnlecker]

Attachment: [medium,l10n] Fix - v2

>Why should the disabled icon not have an updated tooltip?
Yes, you are right. 
Even when adding a row, the tooltip was not updated. Fixed this too.

Comment 9

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 436996 [details] [diff] [review]
[medium,l10n] Fix - v2

Not quite working for me:

* Open the attendee dialog
* add some new attendees
* Change the attendee status by clicking on the icon next to the email address.
* Mouse over the icon

Actual:

The tooltip is still "Required Attendee"

Expected:

Should correspond to the icon (i.e optional attendee).

Comment 10

[m.zahnlecker]

Attachment: [medium,l10n] Fix - v3

Yes, it was just one missing line, because the code got rebuilt pretty much.
Attatched the fixed patch.

Comment 11

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 439890 [details] [diff] [review]
 [medium,l10n] Fix - v3

Yes, much better. Another minor issue I just noticed is how you compose the l10n string. Instead of appending the role, I think its better to add this part to the l10n string. I've fixed this locally so no further action is required.

r=philipp

Comment 12

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/ed1ca157bf2b>
-> FIXED