Closed Bug 525430 Opened 10 years ago Closed 10 years ago

Task view doesn't show the attachments for selected tasks

Categories

(Calendar :: Lightning Only, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bv1578, Assigned: bv1578)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Reproducible:
always

Steps to reproduce:
1. create a new task or open an existing one and add to it one or more 
   attachments (URLs);
2. switch to task mode;
3. select the event just created in the task list.

Actual Results:
below the description area there isn't any indication of the attachments just added to the task.

Expected Results:
below the description area should exist a list with all task's attachments.

Additional Information:
in this code line  http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-task-view.js#156

'item.getProperty("URL")'returns a null value instead of the attachment URL.
Attached patch proposal (obsolete) — Splinter Review
Proposal for displaying more attachments in task view.
I don't know if it needs a UI review because the behavior it's slightly different from the previous. Actually I can compare only with 0.8 version, where only one URL can be added, because with 0.9 one I can't make appear the attached URLs in task view unless the task has been created with 0.8 version (not sure if it's a problem on my Thunderbird or it was a Lightning 0.9's issue.
With this patch the URLs are on different rows, one URL on one row. If there are more URLs, a scrollbar appears on the right side (see next screenshot).
Attachment #409327 - Flags: review?(mschroeder)
Status: NEW → ASSIGNED
Comment on attachment 409327 [details] [diff] [review]
proposal

>+            while (attachmentRows.lastChild) {
>+                attachmentRows.removeChild(attachmentRows.lastChild);
>+            }
We have a helper for this, you can use removeChildren(attachmentRows); (from calendar-ui-utils)


>+            let attachments = item.getAttachments({});
>+            if (displayElement("calendar-task-details-attachment-row", attachments != "")) {
doesn't getAttachments give you an array? Why are you comparing with an empty string here?


r=philipp

I'd appreciate a new patch for checkin.
Attachment #409327 - Flags: review?(mschroeder) → review+
Comment on attachment 409330 [details]
Screenshot with 4 URLs attached to the task

Setting ui-review, but this is quite trivial so I think its fine.

Oh one thing I noticed, I think we should make "attachments:" lowercase, to match the other labels (i.e title).

Please also include that in the next patch.

A different issue, but we should really get those buttons (i.e priority) looking decent. Could you check if we have a bug on that and if not file one? I'd appreciate!
Attachment #409330 - Flags: ui-review?(clarkbw)
> but we should really get those buttons (i.e priority) looking decent.

see Bug 470824
Attached patch proposal - v2Splinter Review
(In reply to comment #4)
> Oh one thing I noticed, I think we should make "attachments:" lowercase, to
> match the other labels (i.e title).

Since the label "Attachments" is the same used in New Event dialog (where the first character must be upper case), I've added another string "attachments" in calendar.dtd file.
I also ask for review because I've aligned the label with the others labels above the description area. To keep the alignment, even when character size changes, I've added an hbox with the same width. Assigning a width to the label (without the hbox) doesn't work as expected.
Attachment #409327 - Attachment is obsolete: true
Attachment #433296 - Flags: review?(philipp)
Attachment #409330 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 409330 [details]
Screenshot with 4 URLs attached to the task

(In reply to comment #4)
> (From update of attachment 409330 [details])
> Setting ui-review, but this is quite trivial so I think its fine.

Looks fine!

> Oh one thing I noticed, I think we should make "attachments:" lowercase, to
> match the other labels (i.e title).

Agreed.

> Please also include that in the next patch.
> 
> A different issue, but we should really get those buttons (i.e priority)
> looking decent. Could you check if we have a bug on that and if not file one?
> I'd appreciate!

If you take a look at the mail headers css you can steal what we've done there.
Comment on attachment 433296 [details] [diff] [review]
proposal - v2

Looks fine to me, r=philipp
Attachment #433296 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/e55527883ee0>

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b2
You need to log in before you can comment on or make changes to this bug.