1496086 - Improve UX in event dialog or tab for attendee or attachment handling

Status: RESOLVED FIXED

(Calendar :: Dialogs, enhancement)

Description

[[:MakeMyDay]]

The event dialog has some UX issues related to the description/attachment/attendee tabs, I'd like to address in this bug:

1. if displaying the event description and adding an attendee or attachment using the toolbar button, you don't see that these item have been added successfully unless you switch the tab.

2. if you open an existing event, you cannot see at a glance whether you have added attendees or attachments to it or not. Again, you would have have to switch to the respective tabpanel.

3. If you edit an existing event with attendees and want to just save your changes but not yet send out an update to the attendees, you have to switch to the attendee tab to uncheck the notify checkbox.

For #1, I suggest that the tab gets switched automatically, while for #2 displaying the number of attachements/attendees would be nice. For #3, it's probably sufficient to move the notify checkboxes from the attendee tabpanel below the tabbox, so that it is displyed always for events.

Comment 1

[[:MakeMyDay]]

Attachment: MinorUXImprovementsForEventDialog-V1.diff

This patch implements the suggested changes. It implements displaying the number of attachments and attendees without requiring string changes, so we could consider to uplift this patch.

Comment 2

[[:MakeMyDay]]

Attachment: AddStringForAttendeesTab-V1.diff

This replaces the handling for displaying the numbers of attachments and attendees by introducing separate strings.

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 9014037 [details] [diff] [review]
MinorUXImprovementsForEventDialog-V1.diff

Yes, this makes sense and this way is compatible with ESR.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9014037 [details] [diff] [review]
MinorUXImprovementsForEventDialog-V1.diff

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

Are you avoiding string changes so we can uplift, or because you feel it is the best solution? If the former, maybe we can uplift what you wrote and use a patch with string changes on c-c that doesn't have any workarounds.

r+wc

::: calendar/lightning/content/lightning-item-iframe.js
@@ +3623,5 @@
> +        if (newLabel.endsWith(":")) {
> +            trailingColon = ":";
> +            newLabel = newLabel.substring(0, newLabel.length - 1);
> +        }
> +        attachmentTab.label = newLabel + " (" + attachments.length + ")" + trailingColon;

Does this code work with RTL locales?

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9014040 [details] [diff] [review]
AddStringForAttendeesTab-V1.diff

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

::: calendar/locales/en-US/chrome/calendar/calendar-event-dialog.properties
@@ +528,5 @@
>  # will be notified on saving
>  sendandcloseMenuLabel=Send and Close
> +
> +# LOCALIZATION NOTE (attendeesTabLabel) - this is a runtime replacement for
> +# event.attendees.label defined in calendar-event-dialog.dtd and used in the 

Whitespace at EOL

@@ +534,5 @@
> +# %1$S - the number of attendee (1-n)
> +attendeesTabLabel=Attendees (%1$S):
> +
> +# LOCALIZATION NOTE (attachmentsTabLabel) - this is a runtime replacement for
> +# event.attachments.label defined in calendar-event-dialog.dtd and used in the 

And again

Comment 6

[[:MakeMyDay]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #4)
> ::: calendar/lightning/content/lightning-item-iframe.js
> @@ +3623,5 @@
> > +        if (newLabel.endsWith(":")) {
> > +            trailingColon = ":";
> > +            newLabel = newLabel.substring(0, newLabel.length - 1);
> > +        }
> > +        attachmentTab.label = newLabel + " (" + attachments.length + ")" + trailingColon;
> 
> Does this code work with RTL locales?

Yes, as far as I understand rtl handling.

rtl strings are also defined with a trailing colon to the right (like ltr), but that is put then to the left when the string is rendered into the ui by some rtl magic (while not changing the order of the letters).

Our other strings which display additional numeric information (like the soondays label in the today pane or the folder names in mail view) are displaying that additional information to the right of the text string when having rtl enabled (you can check this with setting intl.uidirection pref from -1 to 1 and restarting TB afterwards).

So even if that would not be correct, it would be cponsistently wrong ;-)

Comment 7

[[:MakeMyDay]]

Attachment: AddStringForAttendeesTab-V2.diff (for c-c only)

Comment 8

[[:MakeMyDay]]

For checkin, the string patch needs to be applied on top of the other (this is for c-c only, for backport just the other one is needed).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b2ec0ac175e5
UX improvements in event dialog and tab for attendee/attachment handling. r=philipp
https://hg.mozilla.org/comm-central/rev/84694c6debe3
Add a separate string for the attendees tab runtime replacement in the event dialog. r=philipp

Comment 10

[Jorg K (CEST = GMT+2)]

Sorry, I had to change the commit message:

Switch to attachement or attendee tab in event dialog after adding an attachment resp attendees, display number of attendees and attachements and always display notification options for events

Much too long, two typos and no idea what 'resp' means.

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to [:MakeMyDay] from comment #8)
> For checkin, the string patch needs to be applied on top of the other (this
> is for c-c only, for backport just the other one is needed).
Which backport, there is no request.

Comment 12

[[:MakeMyDay]]

Comment on attachment 9014037 [details] [diff] [review]
MinorUXImprovementsForEventDialog-V1.diff

> Are you avoiding string changes so we can uplift, or because you feel it is the best solution? If the former,
> maybe we can uplift what you wrote and use a patch with string changes on c-c that doesn't have any workarounds.

Yes, this patch is intended for uplift. I just missed to request it directly.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9014037 [details] [diff] [review]
MinorUXImprovementsForEventDialog-V1.diff

Too late for 63/6.5 beta now.

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9014037 [details] [diff] [review]
MinorUXImprovementsForEventDialog-V1.diff

Ok, we can take this.

Jörg, thanks for worrying about the commit message. I think it would have been better to ask about this than just change it though. The one you picked is very generic and doesn't really explain what the patch actually does. Its done noe though, we can keep it for the uplift.

Comment 15

[Jorg K (CEST = GMT+2)]

TB 60.5 ESR, Cal. 6.2.5:
https://hg.mozilla.org/releases/comm-esr60/rev/086f4baaf51b4ff9ca5c769209fc039fa7ffcd03

Target needs to be set to 6.2.5. Philipp?