Closed Bug 620221 Opened 9 years ago Closed 9 years ago

Task View: labels for start/due date don't fit into Thunderbird design

Categories

(Calendar :: Tasks, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: mozilla)

Details

(Whiteboard: [needed beta][has l10n impact])

Attachments

(2 files, 2 obsolete files)

Attached image Screenshot
The display of task details in task mode differs:
The labels "Start Date:" and "Due Date:" with capitals and colons differ from other labels here and from the style Thunderbird uses for labels in the Message Pane (small letters, no colons).
The labels should be "start date" and "due date".

Seen with recent builds of Lightning 1.0b3pre + 1.1a1pre.
Attached patch proposed patch (obsolete) — Splinter Review
If this patch is ok, can you please check it in, I can't do this myself.
Assignee: nobody → mozrob
Attachment #498604 - Flags: review?(philipp)
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
I missed that the strings I touched in the patch are also used for the summary dialog - there we need capitals and colons for the labels. :-( Tricky.
Thanks Decathlon for pointing this out.

Nevertheless: As a first step I would change the labels as proposed above (from my personal point of view the task view is the more prominent part of the UI).

Then we would need a followup bug for creating/using separate strings for the summary dialog.
I'd rather see this fixed in one step, rather than burdening the loclizers with changing a further string, just to change it again in the next release.

I haven't spent much time looking at the code here, but possibly we could use a xml attribute and entities to set the labels, i.e

<row class="item-date-row"
     taskStartLabel="..." eventStartLabel="..."
     taskEndLabel="..." eventEndLabel="..." ...>

Robert, do you think you could put together a patch here? Note that there are 2 bugs before string freeze that are already in review, so if you want this to be part of the next beta, I'd appreciate if you could proceed beforehand.
(In reply to comment #3)
Huh, I guess I can't do this in time then.

Adding new entitites just to calendar-task-view.xul won't be enough because (IIUC) label and value are inserted by calendar-item-bindings.xml at once. I wouldn't know how to tell it not to add the label but only the value.

Or did you mean we should get rid of the labels in calendar-item-bindings.xml and add them to calendar-task-view.xul _and_ calendar-summary-dialog.xul directly? That might work, but I'd need lots of trial and error to patch this myself...

Doesn't seem trivial anymore, reassigning to nobody for now.
Assignee: mozrob → nobody
Status: ASSIGNED → NEW
Attachment #498604 - Attachment is obsolete: true
Attachment #498604 - Flags: review?(philipp)
I was rather thinking to get the correct strings from xml attributes inside the binding. Instead of calling calGetString, you use (semi-pseudo-code):


let attribute;
if (mode == start) {
  if (cal.isTodo(item)) {
    attribute = "taskStartLabel";
  } else {
    ...
  }
} else if (mode == end) {
  if (...)
  ...
}

label.setAttribute(this.getAttribute(attribute)) ;

Then, inside calendear-task-view, where the binding is instanciated, you add the xml attributes as suggested in comment 3. The same goes for calendar-summary-dialog, but with different attribute values.

The attribute values should be entities, i.e something like &calendar.summary.taskStart.label; (for the name, match similar entities if possible).
Attached patch patch v2 (obsolete) — Splinter Review
I really shouldn't try things I don't fully understand, but that's how I got it to work as expected now - maybe it even is somehow what was meant by comments #c3 and #c5?
Attachment #512339 - Flags: review?(philipp)
Comment on attachment 512339 [details] [diff] [review]
patch v2

Yes, thats almost what I expected, I think you are doing fine!


>diff --git a/calendar/base/content/calendar-task-view.xul b/calendar/base/content/calendar-task-view.xul
>--- a/calendar/base/content/calendar-task-view.xul
>+++ b/calendar/base/content/calendar-task-view.xul

>+                <row class="item-date-row"
>+                     id="task-start-row"
>+                     mode="start"
>+                     taskViewStartLabel="&calendar.task-details.start.label;"
If you change this to taskStartLabel...

>+                     align="end"/>
>+                <row class="item-date-row"
>+                     id="task-due-row"
>+                     mode="end"
>+                     taskViewDueLabel="&calendar.task-details.due.label;"
...and this to taskDueLabel...

>diff --git a/calendar/base/content/dialogs/calendar-summary-dialog.xul b/calendar/base/content/dialogs/calendar-summary-dialog.xul
>--- a/calendar/base/content/dialogs/calendar-summary-dialog.xul
>+++ b/calendar/base/content/dialogs/calendar-summary-dialog.xul

>+          <row class="item-date-row"
>+               id="item-start-row"
>+               mode="start"
>+               taskStartLabel="&read.only.task.start.label;"
>+               eventStartLabel="&read.only.start.label;"
...and keep the labels and attribute names the way they are here...
>+               align="start"/>
>+          <row class="item-date-row"
>+               id="item-end-row"
>+               mode="end"
>+               taskDueLabel="&read.only.task.due.label;"
>+               eventEndLabel="&read.only.end.label;"
...and here...


...Then it should be sufficient if...
>+                      if (this.hasAttribute("taskViewStartLabel")) {
>+                          var label = this.getAttribute("taskViewStartLabel");
>+                      } else {
>+                          var label = this.getAttribute("taskStartLabel");
>+                      }
...you use  |label = this.getAttribute("taskStartLabel");| instead of this if-else-block.

>                   } else {
>+                      var label = this.getAttribute("eventStartLabel");
While this works since its "var", it would be more clear if you define |label| outside of the if (isToDo...) block with |let label;| and then set the label in this if-else-block



>+<!ENTITY read.only.start.label           "Start Date:">
I'd suggest using read.only.event.start.label here, since its at least theoretically possible that we get new item types.



r=philipp with that fixed, thanks for taking care! If you have time to create a new patch with these changes in the next few days, we can check it in before string freeze.
Attachment #512339 - Flags: review?(philipp) → review+
(In reply to comment #7)
I've updated the patch according to your comments. Some notes:

> [snip]
> ...Then it should be sufficient if...
> >+                      if (this.hasAttribute("taskViewStartLabel")) {
> >+                          var label = this.getAttribute("taskViewStartLabel");
> >+                      } else {
> >+                          var label = this.getAttribute("taskStartLabel");
> >+                      }
> ...you use  |label = this.getAttribute("taskStartLabel");| instead of this
> if-else-block.
I changed this for taskDueLabel, too.

> While this works since its "var", it would be more clear if you define |label|
> outside of the if (isToDo...) block with |let label;| and then set the label in
> this if-else-block
I changed this not only for the start, but also for the end/due part.

> >+<!ENTITY read.only.start.label           "Start Date:">
> I'd suggest using read.only.event.start.label here, since its at least
> theoretically possible that we get new item types.
Done, and also changed read.only.end.label to read.only.event.end.label.

Updated patch will follow.
Assignee: nobody → mozrob
Attachment #512339 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Putting on the blocker list so I don't forget this one :)
Flags: blocking-calendar1.0+
Whiteboard: [needed beta][has l10n impact]
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/87fb9473a51d>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/e5b224bac0c2>
a=philipp
Target Milestone: Trunk → 1.0b3
You need to log in before you can comment on or make changes to this bug.